my $foo = $bar if $baz; # UNDEFINED!

[jw9]

Hallo allerseits,

bin gerade dabei, 00_TUL.pm zu debuggen und habe dabei festgestellt, dass das im Betreff genannte Konstrukt

Code Auswählen Erweitern


  my $foo = $bar if $baz;


kreuz und quer von sehr vielen Modulen verwendet wird.

Dieses Konstrukt ist OFFIZIELL nicht zulässig. Ich zitiere aus "perldoc perlsyn":

NOTE: The behaviour of a "my", "state", or "our" modified with a statement
modifier conditional or loop construct (for example, "my $x if ...") is
undefined. The value of the "my" variable may be "undef", any previously
assigned value, or possibly anything else. Don't rely on it. Future
versions of perl might do something different from the version of perl you
try it out on. Here be dragons.

[jw9]

In diesen Modulen ist das fehlerhafte Konstrukt enthalten:

Code Auswählen Erweitern


$ find -name '*.p[lm]' -print0 |xargs -0 perl -ne '$/=";"; print qq=$ARGV\n= if /my[^a-zA-Z_0-9].*?=.*?[^a-zA-Z0-9\$]if[^a-zA-Z0-9]/ ' |sort|uniq -c|sort -rn
     68 ./FHEM/00_SONOS.pm
     30 ./FHEM/93_DbRep.pm
     30 ./contrib/DS_Starter/93_DbRep.pm
     10 ./FHEM/22_HOMEMODE.pm
      8 ./FHEM/Meta.pm
      8 ./FHEM/10_MQTT_GENERIC_BRIDGE.pm
      8 ./FHEM/10_EnOcean.pm
      7 ./FHEM/93_DbLog.pm
      7 ./FHEM/39_Talk2Fhem.pm
      7 ./contrib/DS_Starter/93_DbLog.pm
      6 ./FHEM/51_Netzer.pm
      6 ./FHEM/21_SONOSPLAYER.pm
      6 ./FHEM/10_ZWave.pm
      5 ./FHEM/Unit.pm
      5 ./FHEM/98_PID20.pm
      5 ./FHEM/38_netatmo.pm
      5 ./contrib/HMCCU/FHEM/88_HMCCU.pm
      4 ./FHEM/88_HMCCU.pm
      3 ./FHEM/98_monitoring.pm
      3 ./FHEM/95_YAAHM.pm
      3 ./FHEM/95_FLOORPLAN.pm
      3 ./FHEM/88_xs1Bridge.pm
      3 ./FHEM/77_SMAEM.pm
      3 ./FHEM/74_Unifi.pm
      3 ./FHEM/72_XiaomiDevice.pm
      3 ./FHEM/72_FRITZBOX.pm
      3 ./FHEM/59_Weather.pm
      3 ./FHEM/59_LuftdatenInfo.pm
      3 ./FHEM/57_Calendar.pm
      3 ./FHEM/53_GHoma.pm
      3 ./FHEM/52_I2C_PCA9685.pm
      3 ./FHEM/52_I2C_DS1307.pm
      3 ./FHEM/37_Spotify.pm
      3 ./FHEM/34_SWAP.pm
      3 ./FHEM/34_ESPEasy.pm
      3 ./FHEM/30_HUEBridge.pm
      3 ./FHEM/21_HEOSPlayer.pm
      3 ./FHEM/01_FHEMWEB.pm
      3 ./FHEM/00_TUL.pm
      3 ./contrib/DS_Starter/77_SMAEM.pm
      3 ./contrib/betateilchen/57_Calendar.pm
      3 ./contrib/98_Sprinkle.pm
      3 ./contrib/98_FileLogConvert.pm
      2 ./FHEM/FritzBoxUtils.pm
      2 ./FHEM/98_telnet.pm
      2 ./FHEM/98_powerMap.pm
      2 ./FHEM/98_archetype.pm
      2 ./FHEM/96_allowed.pm
      2 ./FHEM/93_RFHEM.pm
      2 ./FHEM/90_SIGNALduino_un.pm
      2 ./FHEM/88_Itach_IRDevice.pm
      2 ./FHEM/76_msgDialog.pm
      2 ./FHEM/71_YAMAHA_NP.pm
      2 ./FHEM/70_VIERA.pm
      2 ./FHEM/55_DWD_OpenData.pm
      2 ./FHEM/52_I2C_PCA9532.pm
      2 ./FHEM/50_TelegramBot.pm
      2 ./FHEM/48_BlinkCamera.pm
      2 ./FHEM/10_EIB.pm
      2 ./FHEM/10_CUL_HM.pm
      2 ./FHEM/00_TCM.pm
      2 ./FHEM/00_Neuron.pm
      2 ./configDB.pm
      1 ./lib/FHEM/SynoModules/SMUtils.pm
      1 ./FHEM/OWX_FRM.pm
      1 ./FHEM/lib/ProtoThreads.pm
      1 ./FHEM/98_todoist.pm
      1 ./FHEM/98_serviced.pm
      1 ./FHEM/98_notice.pm
      1 ./FHEM/98_JsonList2.pm
      1 ./FHEM/98_inotify.pm
      1 ./FHEM/98_IF.pm
      1 ./FHEM/98_HMtemplate.pm
      1 ./FHEM/98_DOIFtools.pm
      1 ./FHEM/98_DOIF.pm
      1 ./FHEM/98_DLNARenderer.pm
      1 ./FHEM/98_autocreate.pm
      1 ./FHEM/92_FileLog.pm
      1 ./FHEM/89_ESPEInk.pm
      1 ./FHEM/74_UnifiVideo.pm
      1 ./FHEM/74_Nmap.pm
      1 ./FHEM/73_MPD.pm
      1 ./FHEM/60_allergy.pm
      1 ./FHEM/52_I2C_LCD.pm
      1 ./FHEM/52_I2C_HDC1008.pm
      1 ./FHEM/45_TRX.pm
      1 ./FHEM/44_ROLLO.pm
      1 ./FHEM/40_RFXCOM.pm
      1 ./FHEM/38_CO20.pm
      1 ./FHEM/38_Broadlink.pm
      1 ./FHEM/36_Shelly.pm
      1 ./FHEM/36_ShellyMonitor.pm
      1 ./FHEM/32_SYSSTAT.pm
      1 ./FHEM/32_speedtest.pm
      1 ./FHEM/30_LIGHTIFY.pm
      1 ./FHEM/26_KM273.pm
      1 ./FHEM/12_HMS.pm
      1 ./FHEM/10_KNX.pm
      1 ./FHEM/10_Itach_IR.pm
      1 ./FHEM/10_FRM.pm
      1 ./FHEM/02_FTUISRV.pm
      1 ./FHEM/00_ZWDongle.pm
      1 ./FHEM/00_SIGNALduino.pm
      1 ./FHEM/00_HMUARTLGW.pm
      1 ./FHEM/00_CUL.pm
      1 ./contrib/YAF/FHEM/YAF/libs/json/JSON/backportPP.pm
      1 ./contrib/betateilchen/51_BBB_BMP180.pm
      1 ./contrib/98_dev_proxy.pm
      1 ./contrib/31_PLAYBULB.pm


Da mag vielleicht der eine oder andere false-positive dabei sein. Bei oberflächlichen überfliegen habe ich aber keine gesehen.

[jw9]

Gibt es denn keine Meinungen hierzu?

[OdfFhem]

@jw9

Vermutlich ist dieser Beitrag nicht in der richtigen Rubrik gelandet.
Er passt vielleicht eher in "FHEM Forum --- FHEM - Entwicklung --- FHEM Development"
oder "FHEM Forum --- FHEM - Entwicklung --- FHEM Development --- Perl Ecke".

Desweiteren würde ein verständlicherer Betreff mehr Aufmerksamkeit erzeugen.

Ich hatte mir Deine Liste mal angeschaut und zufällig zwei selbstgenutzte Dateien ausgewählt:
- 02_FTUISRV.pm enthält keine "schädliche" Zeile
- 92_FileLog.pm enthält genau eine "schädliche" Zeile.

Vielleicht sollte man an so einem zentralen Modul mal aufzeigen, was "falsch" bzw. "nicht ganz richtig" ist und was man stattdessen tun sollte/müsste ...

Fraglich ist aber, ob es da tatsächlich zu einem Problem kommen kann oder kommen wird. Weit verbreitet ist zwar die Meinung

... its not allowed and can result in undesired behaviour (e.g. crashing). Well, it won't crash, but it could ...

, aber bis jetzt hat man ja scheinbar Glück gehabt ...

[rudolfkoenig]

Gibt es denn keine Meinungen hierzu?

Doch: vielen Dank fuer die Arbeit, das Aendern meiner Module ist auf meinem TODO, ist aber leider etwas nach unten gerutscht.
Wenn wir eine sichere Methode fuers Pruefen haetten, wuerde ich das auch in unseren SVN-Hook einbauen.

Dieses Thema gehoert nach "FHEM-Development" (einfach Zugang beantragen), Wunschliste lese ich (wie vmtl. etliche Developer auch) nur zufaellig. Keine Ahnung, wer diesen Forumsbschnitt angelegt hat, und wozu.

, aber bis jetzt hat man ja scheinbar Glück gehabt ...

Mich hat es schon gebissen. Die Probleme sind nicht offensichtlich: Fehlermeldungen gibt es keine, nur die Ergebnisse sind je nach input falsch.

[Beta-User]

Auch von meiner Seite: Danke für den Schubser.

Betr. MQTT_GENERIC_BRIDGE siehe https://svn.fhem.de/trac/changeset/23652/
(Falls du beim grep noch was findest: könnte sein, dass es die eine oder andere auskommentierte Zeile gibt, auf die das (noch) matcht).

[Christoph Morrison]

Gibt es denn keine Meinungen hierzu?

Ich hab mal ein paar Issues auf Github aufgemacht ...

[jw9]

Vermutlich ist dieser Beitrag nicht in der richtigen Rubrik gelandet.

Er passt vielleicht eher in "FHEM Forum --- FHEM - Entwicklung --- FHEM Development"
oder "FHEM Forum --- FHEM - Entwicklung --- FHEM Development --- Perl Ecke".

Auf "Development" hatte ich keine Schreibrechte und die Hürden welche zu erhalten erschienen mir doch sehr hoch...

Desweiteren würde ein verständlicherer Betreff mehr Aufmerksamkeit erzeugen.

Hmmm... Ja... Ich wollte halt möglichst präzise sein...

Ich hatte mir Deine Liste mal angeschaut und zufällig zwei selbstgenutzte Dateien ausgewählt:
- 02_FTUISRV.pm enthält keine "schädliche" Zeile

Ja, dass da false-positives sein können hatte ich geschrieben. einen kompletten perl-Parser in einem regex unterzubringen ist nicht ganz einfach  In diesem Fall war die Sequenz "-if\s" innerhalb des regex der Auslöser für den false-positive.

Ich schätze mal, dass von den 452 Hits wohl 5% bis 10% false-positives sein könnten.

- 92_FileLog.pm enthält genau eine "schädliche" Zeile.

Und genau um solche Zeilen geht es ja...

Vielleicht sollte man an so einem zentralen Modul mal aufzeigen, was "falsch" bzw. "nicht ganz richtig" ist und was man stattdessen tun sollte/müsste ...

Naja, das hatte ich ja schon im ersten Beitrag dieses Threads aus der Doku zitiert...

Fraglich ist aber, ob es da tatsächlich zu einem Problem kommen kann oder kommen wird. Weit verbreitet ist zwar die Meinung, aber bis jetzt hat man ja scheinbar Glück gehabt ...

Das Problem ist, dass der Wert der Variablen nicht vorhersagbar ist, wenn die Bedingung unwahr ist. Wenn dann mit "zufälligen" Werten weitergearbeitet wird, kann ales mögliche rauskommen. Das Verhalten kann sich dann auch mit der perl-Version verändern.

Im Übrigen trifft das nicht nur für "if" zu sondern auch auf "unless" und loop-Konstrukte. Wobei loop-Konstrukte wohl kaum jemand im Zusammenhang mit einer Variablen-Deklaration verwenden würde.

Hier nochmal der Einzeiler erweitert, so dass er auch auf "unless" matcht und die verdächtige Stelle ausgibt:

Code Auswählen Erweitern


find /opt/fhem -name '*.p[lm]' -print0 |xargs -0 perl -ne '$/=";"; s/^\n//m;print qq=$ARGV:\n$_\n---\n= if /my[^a-zA-Z_0-9].*?=.*?[^a-zA-Z0-9\$](if|unless)[^a-zA-Z0-9]/ '|less


Oh, und dabei stolpert man auch über Highlights wie

Code Auswählen Erweitern


  if(defined ($streams)){
    my @streams=@{$streams} unless not defined ($streams);
  [ ... ]

Wobei dieser Ausschnitt unbedenklich ist im Sinne dieses Threads. Aber doppelte Verneinung und doppelte Abfrage sind auch nicht gerade zuträglich für Verständlichkeit.

[jw9]

Wenn wir eine sichere Methode fuers Pruefen haetten, wuerde ich das auch in unseren SVN-Hook einbauen.

Ich denke nicht dass es mit halbwegs vertretbarem Aufwand möglich ist eine Methode zu entwerfen die sicher genug für einen Hook wäre.

Die einzige wirklich sichere Methode wäre, wenn der perl-Interpreter selbst warnings werfen würde.

Soweit ich weiss gibt es dafür jedoch keine Möglichkeit. Eigentlich schade, wenn man bedenkt, dass es ein sehr weit verbreitetes Konstrukt ist.

[Beta-User]

Auf "Development" hatte ich keine Schreibrechte und die Hürden welche zu erhalten erschienen mir doch sehr hoch...

? Wäre eine einfache Anfrage, und es würde ggf. auch ausreichen, wenn du dich als "Tester" melden wolltest, falls dir der (in der Sache wenig aussagefähige) Titel "Developer" nicht gefällt...

[Wzut]

Das Thema hatte IMHO doch auch RichardCZ letztes Jahr an den Pranger gestellt und ich dachte es überall erschlagen zu haben , aber

Code Auswählen Erweitern

1 ./FHEM/73_MPD.pm
THX4Info, wird demnächst erledigt.

Edit : Developer wäre wirklich der bessere Platz, habe es eben auch nur durch Zufall gefunden ....

[rudolfkoenig]

Ich habe meine Module geaendert, alles der Sorte "my $xx = ... if(...)" umgebaut.
Soweit ich sehe, habe ich durch die Aenderungen nirgendwo ein Problem gefixt.

00_CUL.pm
00_ZWDongle.pm
01_FHEMWEB.pm
10_ZWave.pm
12_HMS.pm
92_FileLog.pm
96_allowed.pm
98_JsonList2.pm
98_autocreate.pm
98_telnet.pm
FritzBoxUtils.pm

[Sidey]

Zunächst mal danke für den Hinweis. Hatte ich so exakt nicht auf dem Radar.
Grundsätzlich leuchtet es mir aber ein, dass eine Variable unter Umständen nicht existiert wenn das in einem IF Block läuft.
Im PBP stand da auch was zu im Kontext von Schleifen etc.

An einem automatisierten Test hatte ich auch gedacht, würde aber erwarten dass perlcritic das bereits detektieren kann.

Die Regex von dir, fängt meiner Ansicht nach folgenden false positive fall:

Code Auswählen Erweitern

my %gets = (  # NameOFCommand =>  StyleMod for Fhemweb, SubToCall if get is executed, String to send to uC, sub called with response, regex to verify response,

Grüße Sidey

[Wzut]

würde aber erwarten dass perlcritic das bereits detektieren kann.

ja das ist mir heute morgen auch erst eingefallen, bei einem ersten Test meckert percritic sogar mit Level 5:

Code Auswählen Erweitern

Variable declared in conditional statement at line 847, column 5.  Declare variables outside of the condition.  (Severity: 5)

[jw9]

Grundsätzlich leuchtet es mir aber ein, dass eine Variable unter Umständen nicht existiert wenn das in einem IF Block läuft.
Im PBP stand da auch was zu im Kontext von Schleifen etc.

Grundsätzlich würde das Problem auch bei Schleifen existieren. In freier Wildbahn halte ich das aber für eher unwahrscheinlich. Wer würde schon auf die Idee kommen etwas wie "my $foo = 'bar' while $baz;" zu schreiben?

Und tatsächlich: wenn ich den regex um "while" und "until" erweitere, habe ich 13 weitere matches, allesamt false-positives.

An einem automatisierten Test hatte ich auch gedacht, würde aber erwarten dass perlcritic das bereits detektieren kann.

Bei perlcritic habe ich sehr gemischte Gefühle. Ich habe mal probehalber einige meiner Projekte durch perlcritic gejagt und Tonnen von Warnings erhalten, die allesamt harmlos/unbegründet waren. Ein Tool das 99% false-positives liefert ist keine echte Hilfe, zumindest nicht wenn man das automatisieren will... Ich muss aber zugeben, dass ich perlcritic in der default-configuration verwendet habe. Vermutlich kann man das so konfigurieren, dass es sinnvollere Ergebnisse liefert.

Die Regex von dir, fängt meiner Ansicht nach folgenden false positive fall:

Code Auswählen Erweitern

my %gets = (  # NameOFCommand =>  StyleMod for Fhemweb, SubToCall if get is executed, String to send to uC, sub called with response, regex to verify response,

Wie gesagt: das ist eine einfache regex und kein vollständiger perl-parser. Eine regex ist grundsätzlich nicht in der Lage sauber zu arbeiten, sobald Klammern und/oder quotes im Spiel sind. Dazu benötigt es einen Stack, den eine regex nuneinmal nicht hat. Und nein, den Kommentar einfach wegclipsen geht nicht, denn der könnte gequotet sein und man geht das Risiko ein, ein true-positive zu verfehlen.

Solange der Anteil an false-positives kleiner als (sagenwirmal) 30% ist, ist es wesentlich effizienter die true-positives abzuarbeiten als das matching-Verfahren zu optimieren. YMMV!