Autor Thema: my $foo = $bar if $baz; # UNDEFINED!  (Gelesen 3412 mal)

Offline jw9

  • New Member
  • *
  • Beiträge: 37
my $foo = $bar if $baz; # UNDEFINED!
« am: 20 Januar 2021, 12:04:16 »
Hallo allerseits,

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

  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":
Zitat
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.

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #1 am: 24 Januar 2021, 21:57:01 »
In diesen Modulen ist das fehlerhafte Konstrukt enthalten:

$ 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.

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #2 am: 12 Februar 2021, 10:27:47 »
Gibt es denn keine Meinungen hierzu?

Offline OdfFhem

  • Sr. Member
  • ****
  • Beiträge: 783
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #3 am: 12 Februar 2021, 11:27:28 »
@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
Zitat
... 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 ...

Offline rudolfkoenig

  • Administrator
  • Hero Member
  • *****
  • Beiträge: 24474
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #4 am: 12 Februar 2021, 11:34:50 »
Zitat
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.

Zitat
, 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.

Offline Beta-User

  • Developer
  • Hero Member
  • ****
  • Beiträge: 15568
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #5 am: 12 Februar 2021, 11:48:56 »
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).
Server: HP-T620@Debian 10, aktuelles FHEM + ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn:MySensors, Weekday-&RandomTimer, Twilight,  AttrTemplate {u.a. mqtt2, mysensors, zwave}

Offline Christoph Morrison

  • Developer
  • Hero Member
  • ****
  • Beiträge: 1767
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #6 am: 12 Februar 2021, 12:24:43 »
Gibt es denn keine Meinungen hierzu?

Ich hab mal ein paar Issues auf Github aufgemacht ...
Gefällt mir Gefällt mir x 1 Liste anzeigen

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #7 am: 12 Februar 2021, 12:40:59 »
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...

Zitat
Desweiteren würde ein verständlicherer Betreff mehr Aufmerksamkeit erzeugen.
Hmmm... Ja... Ich wollte halt möglichst präzise sein...

Zitat
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.

Zitat
- 92_FileLog.pm enthält genau eine "schädliche" Zeile.
Und genau um solche Zeilen geht es ja...

Zitat
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...

Zitat
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:
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
  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.

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #8 am: 12 Februar 2021, 12:48:45 »
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.

Offline Beta-User

  • Developer
  • Hero Member
  • ****
  • Beiträge: 15568
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #9 am: 12 Februar 2021, 12:56:30 »
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...
Server: HP-T620@Debian 10, aktuelles FHEM + ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn:MySensors, Weekday-&RandomTimer, Twilight,  AttrTemplate {u.a. mqtt2, mysensors, zwave}

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 4533
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #10 am: 12 Februar 2021, 17:16:51 »
Das Thema hatte IMHO doch auch RichardCZ letztes Jahr an den Pranger gestellt und ich dachte es überall erschlagen zu haben , aber
1 ./FHEM/73_MPD.pmTHX4Info, wird demnächst erledigt.

Edit : Developer wäre wirklich der bessere Platz, habe es eben auch nur durch Zufall gefunden ....
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline rudolfkoenig

  • Administrator
  • Hero Member
  • *****
  • Beiträge: 24474
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #11 am: 12 Februar 2021, 21:32:18 »
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

Offline Sidey

  • Developer
  • Hero Member
  • ****
  • Beiträge: 2594
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #12 am: 12 Februar 2021, 21:48:55 »
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:

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

Signalduino, Homematic, Raspberry Pi, Mysensors, MQTT, Alexa

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 4533
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #13 am: 13 Februar 2021, 08:41:44 »
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:
Variable declared in conditional statement at line 847, column 5.  Declare variables outside of the condition.  (Severity: 5)
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #14 am: 13 Februar 2021, 13:19:04 »
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.

Zitat
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.

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

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!