nicht angemeldet
Abend CPAN
Ich vermutete datenintegritäts- und sicherheitskritische Fehler. Diese sind tatsächlich vorhanden. Wenn ein Feldinhalt (z.B. "Nachricht") ein Semikolon enthält, verrutschen die Inhalte und werden nicht mehr den korrekten Variablen zugeordnet, die Ausgabe wird ruiniert. Inhalte werden vor der Ausgabe nicht kodiert, so dass ein Angreifer beliebigen schadhaften Code einschleusen kann. Dies ist ernst und gehört dringend verbessert! Ich spare mir die Details, bei der kompletten Überarbeitung bleibt nichts mehr vom fehlerhaften übrig.
Sorry, das ist absoluter quatsch! Du siehst ja nur den "Reader", und der liest bekanntlich, und schreibt nichts in die csv! Der "Writer" ist sehr sicher. Und da nur der "Writer"in die csv-Datei schreiben darf, brauche ich beim "Reader" keine Semikolons fürchten. Semikolons werden in Gatterzeichen umcodiert. Und beim auslesen wird aus dem Gatterzeichen wieder ein Semikolon. Eine Zeile hat sich kurz nach dem posten des reader-Codes geändert:
$DB_nachricht =~ s/#/;/g;
Die war glaub oben noch nicht drin!
Nun zum Design auf hoher Abstraktion: du benutzt keine bewährten Methoden und Mittel, um deine Ziele umzusetzen, vermutlich aus Unkenntnis, dass es sie gibt (nämlich: Datenbanken und Vorlagen). Dadurch wird dein Code unnötig lang und fehleranfällig, da du besagte Methoden und Mittel selber schlecht umsetzt, statt auf vorhandenen Code zurückzugreifen.
Das ist Absicht. Ich will kein TEXT::CSV oder DBD::CSV. Sondern nur selbstgemachtes!
Es folgt Designkritik zu konkreten Punkten:
#!/usr/bin/perl -w
Dieser Schalter ist überholt. Du benutzt bereits den Ersatz, das lexikalische Pragma warnings. Entferne den Schalter!
Dieser Schalter wird von mir, ich schätze mal sicher schon die letzten 5 Jahre, benutzt. Ohne das es Anlass zur Sorge gegeben hätte.
Du schreibst ein CGI-Programm, es führt also Code im Auftrag anderer Leute (also der Benutzer deines Webangebots) aus. Schalte Taintchecks mittels -T ein!
Das kenne ich nicht -> muss nachlesen, für was das ist.
###############################################################################
Solche Verzierungen taugen als Illuminierung mittelalterlicher Manuskripte, gehören aber nicht in den Quellcode. Entferne sie! Wenn du Unterteilung von logischen Codeeinheiten sichtbar machen möchtest, benutze Subroutinen und rücke sie ein!
Ich steh total auf solche Hinweise im Code. Wenn ich ein Jahr später was ändern muss, weiss ich gleich, um was es im nächsten Block geht.
guest-reader.pl: Version 1.00
Dieses Metadatum gehört nicht in einen Programmiererkommentar. Benutze die reservierte Packagevariable $VERSION!
our $VERSION = '1.00';
Developed started in: 02.08.2012
Finished in: 03.08.2012
Das gehört nicht in den Code. Dein Versionskontrollsystem gibt bereits Aufschluss über diese Daten. Entferne sie!
Da bin ich anderer Meinung *g*. Das wer, wie und wann gehört in den Code! A) Wie willst du später beweisen, wenn einer deinen Code kopiert und nutzt, das es dein Code ist? Es gibt ja Blindkopierer ohne Codeahnung. B)Wenn sich einer später den Code anguckt, sieht er gleich, wer der Vater des Codes war. Beim C64 kamm das auch immer in den Code!
Programmed by: XXXXXXXXXXXXXXX
Dieses Metadatum gehört nicht in einen Programmiererkommentar, sondern in die Dokumentation. Benutze den dafür in Pod vorgesehenen Absatz!
Die Doku liest eh keiner ;-)
Aber zur Beruhigung: Ist alles auf Papier ausgedruckt und nochmal auf CD gebrannt - inklusive Anleitung - Ja, da bin ich ganz akurat.
Extra-Skalar-Group (ESG) / Skalare
So was gibt es nicht. Hier fehlt Dokumentation zum Verständnis!
Da hast du recht. Das ist, weil im Writer-Program diese Skalare extra für die Speicherung benötigt wurden. Und weil Porgammierer faul sind, hab ich die grad so übernommen.
Du hast mehrere Skalarvariablen, deren Namen mit dem gleichen Präfix anfangen.
Klar, DB-steht für _D_aten_B_ank (Skalar) Einer meiner bewährten Methoden um zu wissen woher der Inhalt ist.
Das sollte immer ein Warnsignal sein, dass hier besser eine Verbundvariable taugen würde. Aggregiere deine Datenfelder in einem Hash!
Das mir dem Hash haben schon mehrere gesagt. Leider kenne ich die nicht, und der Umgang mit Ihnen (Hash) ist mir unbekannt ->muss das wanwenden, das ich kenne.
Auch hat Deutsch nichts in Programmen verloren. Schreibe den Code, insbesondere die Bezeichner, auf englisch!
Das Prigramm ist für Deutschland, Österreich und Schweiz(D-CH). Da kann der Inhalt auch ruhig deutsch sein. Bei Internationalen Projekten pflichte ich dir aber bei.
print CGI->header('text/html');
Die Zeichenkodierung fehlt. Die empfohlene Kodierung für das Web ist UTF-8. Halte deine Daten UTF-8 vor und gebe sie so kodiert aus!
Das ist doch schon passiert. Das Script ergänzt nur die Html-Seite.
Read-Out-Engine / Buchausleser
Hier gibt es keine Engine. Das Wort hat eine spezielle Bedeutung, nicht jeder Code ist eine Engine. Ersetze den Kommentar mit einer benannten Subroutine!
Das ist der Hinweis für mich, dass jetzt der Teil im Programm folgt, der die Daten holt. Dieser Teil verrichtet arbeit ->Read-Out-Engine
open(LESER, "<XXXXXXX/XXXXXXX.csv") or die "ERROR: Unable to open the Message-file during read-access!";
Das ist unidiomatisch. Verwende immer die Drei-Argumente-Variante von open! Verwende lexikalische Dateihandles! Die Fehlernachricht sagt nicht, warum das Öffnen fehlschlägt. Überlasse autodie die Fehlerprüfung und Erzeugung der Fehlernachricht!
my ($DB_nick, $DB_nachricht, $DB_zaehler, $DB_gbrgelesen, $DB_IPAdresse, $DB_Monatstag, $DB_Monat, $DB_Jahr, $DB_Stunden, $DB_Minuten, $DB_Sekunden) = split(/;/,$zeile);
Dies gibt vor, ein CSV-Parser zu sein. Es funktioniert aber nicht, weil er Escapingregeln missachtet. Benutze einen korrekten Parser, z.B. Text::CSV!
Das gibt nicht vor irgendwas zu sein. Es ist einfach ein Stück Code, dass Daten holt und ausgibt. Und das war einene heiden Arbeit, bis das syntaktisch fehlerfrei lief.
$counter = ($counter + 1);
Das ist unidiomatisch. Benutze den Inkrementoperator!
$counter++;
Deines ist kürzer, meines menschlicher - und letzterer muss den Code bearbeiten.
unshift (@Collector, $counter, $DB_nick, $DB_nachricht, $DB_zaehler, $DB_gbrgelesen, $DB_IPAdresse, $DB_Monatstag, $DB_Monat, $DB_Jahr, $DB_Stunden, $DB_Minuten, $DB_Sekunden);
Es ist eine enorme Speicherverschwendung, erst alle Daten in eine riesige Datenstruktur einzusammeln und dann darüber zu iterieren, um die Ausgabe zu befüllen. Ab einer gewissen Menge von Daten wird dieses Programm nicht mehr funktionieren, die Experten sprechen davon, dass es nicht skaliert. Stattdessen befülle die Ausgabe stückchenweise mit jeweils nur einem Datensatz auf einmal!
Hier hab ich lange überlegt, ob ich einen Satz bearbeite, dann Html ausgebe und dann denn nächsten bearbeite. Da ich denke, das es besser ist, wenn perl die Html-Daten in einem rutsch ausgibt, hab ich zuerst alle daten bearbeitet, und dann am stück ausgegeben. Für ein Gästebuch ausreichend. Für Firmen mit Artikelnummern, Lieferschein-Nummer, Auftrags-Nummern usw. ist der Bearbeitungsteil (aus Speichergründen) natürlich klein zu halten - Full ACK. Dann würd ich mich aber in DBD::CSV einarbeiten, weil dass wär mir dann zuviel.
foreach ( my $i=0; $i<@Collector; $i+=12) {
my ($counter, $DB_nick, $DB_nachricht, $DB_zaehler, $DB_gbrgelesen, $DB_IPAdresse, $DB_Monatstag, $DB_Monat, $DB_Jahr, $DB_Stunden, $DB_Minuten, $DB_Sekunden)=@Collector[ $i .. $i+11 ];Es befindet sich hier magische Zahlen. Benutze Konstanten! Wenn du dein Datenschema änderst, z.B. eine neue Spalte hinzufügst, so braucht nur die Konstante angepasst werden.
exit;
Das ist überflüssig.
Dachte ich schon - ich habs zur Sicherhet platziert *schande über mein Haupt*
Es folgt nun das verbesserte Programm unter Einhaltung der derzeit als bewährt erachteten Methoden und Mittel. Ein Großteil der vorher genannten Ratschläge ist hiermit hinfällig. [...]
Das mag "stilistisch" besser sein - für mich aber in _weiten_ Teilen nicht mehr lesbar. -> Ich versteh dann mein eigenes Programm nicht mehr.
es wäre nicht optimal strukturiert
Du halst dir mit SSI Ärger auf, [...]
SSI ist absolut genial. Ich möchte darauf nie mehr verzichten müssen. Geschmaksache!
Puh...
Soweit bis hierher
Gruss HP-User