C-Programm zerpfluecken (was: Re: Zufallssignaturen unter E-Mails)
Moin, Am Donnerstag, 7. Oktober 2004 20:00 schrieb Daniel Lord: [...]
kleines C Programm (siehe unten)
Du hast es so gewollt ;-) Ich hoffe Du nimmst es mir nicht übel, wenn ich mal ein wenig in Deinem Code rumwildere.
[...] #include #include #include #include #include #define MAXSIGS 10000 #define SIGWIDTH 80+5 /* more is abuse */
Setze defines immer in Klammern, wenn sie nicht nur aus einem einzigen konstanten Wert bestehen. Du weisst nie, wo sie später verwendet werden ;-) char ups[SIGWIDTH*10]; /* expandiert zu ups[80+5*10]=ups[130] */
#define SIGLEN 6*(80+5) /* ugly and has to be less than
warum nicht so (gleich mal mit mir genehmen Werten ;-): #define SIGWIDTH (72+1) #define SIGLEN (4*SIGWIDTH)
#ifdef JUSTMYEXE #define MYSIGFILE "/home/user/.mutt/randomsigs" #endif [...] short emptyline (char *string) { unsigned short l;
if (string[0] == '\n') return 0;
Unnötig, wird mit der for-Schleife auch erledigt.
for (l=0; l < (strlen(string)-1); l++) {
Nur eine Kleinigkeit: Du gehst mit l bis zu strlen(string)-1. Warum? Die Berechnung ist unnötig, das letzte Zeichen ist bei fgets eh ein \n (es sei denn, die Zeile ist zu lang - siehe unten) - ändert also nichts am Ergebnis. strlen(string) reicht.
if ((string[l] != ' ') && (string[l] != '\t')) { return 1; } } return 0; }
Ich würde es wie unten beschrieben machen.
int main (int argc, char *argv[]) { FILE *fd;
unsigned long signatur=1; /* overkill at the moment */ unsigned long randnum=-1;
Ist beides IMHO Overkill. Die Funktion rand(), die Du unten benutzt, liefert nur int. Damit kann auch ein rand()%signatur nie mehr als einen int liefern.
unsigned long count=0; unsigned char tmp[SIGWIDTH];
struct sig_data { unsigned char *data; }; struct sig_data sig[MAXSIGS];
Warum eine Struktur, wenn Du nur ein Array aus *char benötigst?
/* Handle the argument */ #ifdef JUSTMYEXE if (argc != 1){ fprintf(stderr," use the source luke\n"); return EXIT_FAILURE; } #else if (argc != 2){ fprintf(stderr," usage: %s randomsig.file\n",argv[0]); return EXIT_FAILURE; } #endif
Diese ifdefs würde ich anders lösen: mailsig_02 [randomsig.file] also wahlweise mit oder ohne Argument aufzurufen (s. u.)
/* Initialise random number generator */ srand (time (0)); /* init random */
/* Open the signaturefile for reading */ #ifdef JUSTMYEXE if ((fd=fopen(MYSIGFILE,"ro")) == NULL){ #else if ((fd=fopen(argv[1],"ro")) == NULL){ #endif
"ro"? - lt. meiner Manualpage gibt es nur "r" für den read-only Modus.
perror("fopen"); return EXIT_FAILURE; }
Ab hier habe ich ein grundsätzliches Problem mit Deinem Code. Warum liest Du die gesamte Datei in den Speicher (und schlägst Dich mit malloc() und den damit verbundenen Risiken rum), nur um die maximale Anzahl von Signaturen zu ermitteln? Ich halte das für unnötig. Meine Version habe ich unten angehängt.
/* Read the signature file and sort it into the sig_data struct */ if ((sig[signatur].data = malloc(SIGLEN+1)) == NULL) { perror("malloc"); goto failed; } sig[signatur].data[0]=0x00;
while (fgets(tmp, SIGWIDTH, fd) != NULL) { if (emptyline(tmp) != 0) { strcat(sig[signatur].data,tmp);
Was passiert, wenn eine Zeile länger als SIGWIDTH-1 ist? Dann fehlt Dir das abschliessende \n am Zeilenende und die aktuelle Zeile wird in der Ausgabe an die davor gehende angehängt. Damit ist Deine Prüfung auf max. Zeilenlänge obsolet. Schlimmer: Du prüfst nicht, wie lang der Inhalt von .data bereits ist. Eine Signatur mit mehr als 6 Zeilen a 84 Zeichen würde wahrscheinlich einen Segmentation Fault bewirken, da in nicht initialisierten Speicher geschrieben wird.
} else { /* Allocate memory for the next signature */ signatur++; if ((sig[signatur].data = malloc(SIGLEN+1)) == NULL) { perror("malloc"); goto failed; } sig[signatur].data[0]=0x00;
Ähm - was machst Du, wenn mehrere leere Zeilen nacheinander kommen? Dann hast Du leere Signaturen.
} }
/* if first malloc failes the app probably segfaults later. but may also end without problems so no exit here. */ failed: fclose(fd);
Hm. Ein "Könnte schiefgehen, muss aber nicht* halte ich für nicht so glücklich. Zumindest solltest Du nach einem abgebrochenen malloc() signatur um 1 verringern, damit Du nicht in uninitialiserte Bereiche kommst.
/* get a random number between 1 and signatur */ randnum = rand()%(signatur)+1;
/* make sure you don't get flamed :) */ if (strlen(sig[randnum].data) > SIGLEN) {
Wie soll das gehen? Du initialisierst doch .data nur mit SIGLEN+1 und wenn die Signatur länger ist, dann gibts oben einen Buffer Overflow. Also besser gleich oben abfangen.
strcpy(sig[randnum].data, "Probably PEBCAK. Signature way to long\n"); }
/* Write random signatur to stdout */ printf("%s",sig[randnum].data);
/* Free the allocated memory */ for (count=1; count <= signatur; count++) { free(sig[count].data); }
Noch ne Kleinigkeit: Warum nutzt Du nicht in Deiner Struktur den Index ab 0? Du würdest Dir eine Menge +1 usw. ersparen :-)
return 0; }
So, nun meine Version. Ich lese die Datei 1,x mal zugunsten einer
einfacheren Programmstruktur (und nehme dafür ein paar
Millisekunden längere Programmlaufzeit in Kauf). Man kann auch da
sicher noch ein paar Dinge optimieren, aber als erster Wurf ;-)
Bevorzugen würde ich aber eigentlich immer ein Modell, bei dem das
Dateiformat der Signaturdatei bereits beim Erzeugen validiert wird
(siehe die anderen Mails im Thread):
<schnipp>
#include
Am Freitag, 8. Oktober 2004 00:54 schrieb Jan Trippler: [...]
short emptyline (char *string) { unsigned short l;
if (string[0] == '\n') return 0;
Unnötig, wird mit der for-Schleife auch erledigt.
for (l=0; l < (strlen(string)-1); l++) {
Nur eine Kleinigkeit: Du gehst mit l bis zu strlen(string)-1. Warum? Die Berechnung ist unnötig, das letzte Zeichen ist bei fgets eh ein \n (es sei denn, die Zeile ist zu lang - siehe unten) - ändert also nichts am Ergebnis. strlen(string) reicht.
if ((string[l] != ' ') && (string[l] != '\t')) { return 1; } } return 0; }
[...]
Ist natürlich Quatsch - Du fragst in der Schleife ja nicht \n ab - ich war in Gedanken schon bei isspace(). -- Linux-Quickies: http://www.jan-trippler.de PingoS: http://www.pingos.org
Hallo, Am Fri, 08 Oct 2004, Jan Trippler schrieb:
Am Donnerstag, 7. Oktober 2004 20:00 schrieb Daniel Lord: [..]
/* Initialise random number generator */ srand (time (0)); /* init random */ /* Initialise random number generator */ srand (time (0));
Ihr wisst, dass ihr damit innerhalb einer Sekunde immer die gleiche
Sig bekommt?
$ cat testrand.c
#include
Moin, Am Freitag, 8. Oktober 2004 14:23 schrieb David Haller:
Am Fri, 08 Oct 2004, Jan Trippler schrieb:
Am Donnerstag, 7. Oktober 2004 20:00 schrieb Daniel Lord:
[..]
/* Initialise random number generator */ srand (time (0)); /* init random */
/* Initialise random number generator */ srand (time (0));
Ihr wisst, dass ihr damit innerhalb einer Sekunde immer die gleiche Sig bekommt?
Das wäre wohl nur für Spammer schlimm ;-) - Ich habs noch nie geschafft, mehr als 1 Mail / Sekunde abzusetzen (gaaanz grob geschätzt ;-) [...]
Ok, bei sigs ist das wohl nicht so schlimm ;)
Jepp.
Generell waere es besser, z.B. mit vier Bytes aus /dev/urandom zu initialisieren:
$ cat testrandom.c #include
#include #include #include #include int main(void) { unsigned int buf; int status; int rfd = open("/dev/urandom", O_RDONLY); if(rfd == -1) { perror(""); exit(errno); } status = read(rfd, &buf, sizeof(rfd)); if( status == -1 ) { perror(""); close(rfd); exit(errno); } if( status != sizeof(rfd) ) { fprintf(stderr, "Short read from '/dev/urandom'\n"); close(rfd); exit(1); } close(rfd); srand(buf); printf("%i\n", rand()); return 0; }
Das wäre IMHO mit Kanonen auf Spatzen geschossen: - Die Wahrscheinlichkeit ist annähernd 0, dass man mehrmals / Sekunde Mails schickt (es sei denn, man ist Spammer) - Wenn die gleiche Signatur 2x hintereinander kommt: na und? - Auch so ist es möglich, dass mehrmals die gleiche Signatur kommt, es sind eben Zufallszahlen - da können schon mal die gleichen Zahlen mehrmals fallen (je geringer die Anzahl der Signaturen, desto höher die Wahrscheinlichkeit). Man muss immer Aufwand und Nutzen gegeneinander abwägen. In diesem Fall ziehe ich Daniels Lösung definitiv vor. Wenn ich einen Programmaufruf so oft starten muss, dass ich mehrmals pro Sekunde einen Zufallswert brauche, dann implementiere ich ihn lieber als persistenten Dienst und initialisiere nur einmal. Jan -- Linux-Quickies: http://www.jan-trippler.de PingoS: http://www.pingos.org
Hallo, Am Fri, 08 Oct 2004, Jan Trippler schrieb:
Am Freitag, 8. Oktober 2004 14:23 schrieb David Haller:
Am Fri, 08 Oct 2004, Jan Trippler schrieb:
Am Donnerstag, 7. Oktober 2004 20:00 schrieb Daniel Lord: [..] Generell waere es besser, z.B. mit vier Bytes aus /dev/urandom zu initialisieren:
[snip Beispielcode]
Das wäre IMHO mit Kanonen auf Spatzen geschossen: - Die Wahrscheinlichkeit ist annähernd 0, dass man mehrmals / Sekunde Mails schickt (es sei denn, man ist Spammer)
Als einzelner: ja, unwahrscheinlich.
- Wenn die gleiche Signatur 2x hintereinander kommt: na und?
Ack. Deswegen mein Kommentar.
- Auch so ist es möglich, dass mehrmals die gleiche Signatur kommt, es sind eben Zufallszahlen - da können schon mal die gleichen Zahlen mehrmals fallen (je geringer die Anzahl der Signaturen, desto höher die Wahrscheinlichkeit).
Ja.
Man muss immer Aufwand und Nutzen gegeneinander abwägen. In diesem Fall ziehe ich Daniels Lösung definitiv vor. Wenn ich einen Programmaufruf so oft starten muss, dass ich mehrmals pro Sekunde einen Zufallswert brauche, dann implementiere ich ihn lieber als persistenten Dienst und initialisiere nur einmal.
Hast du daran gedacht, dass das Programm evtl. mehrere User "fast gleichzeitig" (innerhalb der selben Sekunde) aufrufen koennten? Das initialisieren aus /dev/urandom erschlaegt das Problem effektiv, ein Signature-Server (oder "persistenter Dienst") hingegen ist wesentlich komplexer zu programmieren. Als Zufallszahl, kann man schlicht die aus /dev/urandom gelesene verwenden. Die ist sowieso "besser". Und fuer echte Zufallszahlen ist rand(3) ja sowieso ungeeignet. rand(3) ist ja noch nichtmal ein guter PRNG (bzw. ist gar kein PRNG?). Brauche ich Zufallswerte, dann implementiere ich keinen Dienst, sondern lese einfach aus /dev/urandom! Denn das ist genau so ein "Dienst", der praktischerweise schon fertig im Kernel implementiert ist. Und brauche ich wirklich gute Zahlen gibt's noch /dev/random. Siehe /usr/src/linux/drivers/char/random.c. Insofern sollte man srand/rand also einfach vergessen, sofern man nicht _reproduzierbare_ Folgen pseudo-zufaelliger Zahlen _benoetigt_. Das letztere, naemlich die Eigenschaft reproduzierbare _Folgen_ von Zahlen zu produzieren, das hat /dev/{u,}random nicht. Wowereit. * Brauchst du sehr gute[1] Zufallszahlen? lies /dev/random. * Brauchst du gute Zufallszahlen? Lies /dev/urandom. * Brauchst du eine zufaellige Folge von zufaellig-aussehenden Zahlen: verwende srand/rand, initialisiert mit einer Zahl aus /dev/urandom. * Brauchst du eine Folge von zufaellig-aussehenden Zahlen: verwende srand/rand, initialisiert mit einer Zahl aus einer anderen Quelle. Fuer Signaturen reicht allerdings sicher der letzte Fall. Da reicht aber auch $RANDOM der bash (und anderer shells?). -dnh [1] kryptographisch gute -- Es fehlt die Glaskugel, die voraussagen kann, welche der anderen Glaskugeln heute am genauesten glaskugelt. -- Christoph Päper in drtm
Am Samstag, 9. Oktober 2004 03:57 schrieb David Haller: [...]
Hast du daran gedacht, dass das Programm evtl. mehrere User "fast gleichzeitig" (innerhalb der selben Sekunde) aufrufen koennten?
Mit der gleichen Signaturdatei? Wir reden doch hier nicht von einem Enterprise-Signaturgenerator. Und selbst wenn das Teil für so einen Zweck eingespannt wird und mehrere User die gleiche Signatur haben - na und?
Das initialisieren aus /dev/urandom erschlaegt das Problem effektiv, ein Signature-Server (oder "persistenter Dienst") hingegen ist wesentlich komplexer zu programmieren.
Ich meinte damit auch nicht einen Signaturserver. Sowas würde ich nie programmieren, weil das für so eine simple Anwendung erst recht mit Kanonen auf Spatzen geschossen wäre. Die Initialisierung mit srand (time (0)) ist für diesen Fall eben _kein_ Problem, weils nicht drauf ankommt.
Als Zufallszahl, kann man schlicht die aus /dev/urandom gelesene verwenden. Die ist sowieso "besser".
Und fuer echte Zufallszahlen ist rand(3) ja sowieso ungeeignet. rand(3) ist ja noch nichtmal ein guter PRNG (bzw. ist gar kein PRNG?). [...]
Das stimmt ja alles - aber nochmal: Für einen simplen Signatur-Selektor ist das völlig überzogen, die Qualität der Zufallszahl ist für diese Anwendung nebensächlich. Ich ziehe es vor, für einfache Probleme auch möglichst einfache Lösungen einzusetzen (KISS). BTW: Man kann auch völlig auf Zufallszahlen verzichten und z. B. per time(0) % signatur die aktuelle Signatur ermitteln - auch das würde schon (bei nicht zu großen Signaturdateien - sagen wir mal <10.000 Einträge) ausreichen. Die Zufälligkeit der Zeitspanne zwischen 2 verschickten Mails ist für diese Anwendung gut genug. Jan -- Linux-Quickies: http://www.jan-trippler.de PingoS: http://www.pingos.org
Hallo Jan und David & Mitleser, On Fri, Oct 08, 2004 at 12:54:05AM +0200, Jan Trippler wrote:
Am Donnerstag, 7. Oktober 2004 20:00 schrieb Daniel Lord: [...]
kleines C Programm (siehe unten)
Du hast es so gewollt ;-) Ich hoffe Du nimmst es mir nicht übel, wenn ich mal ein wenig in Deinem Code rumwildere.
ich lerne gerne was dazu.
[...] Setze defines immer in Klammern, wenn sie nicht nur aus einem einzigen konstanten Wert bestehen. Du weisst nie, wo sie später verwendet werden ;-) char ups[SIGWIDTH*10]; /* expandiert zu ups[80+5*10]=ups[130] */
der Rat ist gut und wird in Zukunft befolgt ;)
#define SIGLEN 6*(80+5) /* ugly and has to be less than
warum nicht so (gleich mal mit mir genehmen Werten ;-): #define SIGWIDTH (72+1) #define SIGLEN (4*SIGWIDTH)
weil das nur mit Klammer geht *g*
unsigned long signatur=1; /* overkill at the moment */ unsigned long randnum=-1;
Ist beides IMHO Overkill. Die Funktion rand(), die Du unten benutzt, liefert nur int. Damit kann auch ein rand()%signatur nie mehr als einen int liefern.
Der Kommentar war auch für beides und noch ein bischen mehr gedacht ;)
struct sig_data { unsigned char *data; }; struct sig_data sig[MAXSIGS];
Warum eine Struktur, wenn Du nur ein Array aus *char benötigst?
Das diente nur der besseren Erweiterbarkeit. Eine Struktur benötigt an sich keinen Speicherplatz. Und da ich das Programm irgendwann evtl. noch um Zusatzfunktionen aufstocken wollte... Deshalb struct.
/* Handle the argument */ #ifdef JUSTMYEXE if (argc != 1){ fprintf(stderr," use the source luke\n"); return EXIT_FAILURE; } #else if (argc != 2){ fprintf(stderr," usage: %s randomsig.file\n",argv[0]); return EXIT_FAILURE; } #endif
Diese ifdefs würde ich anders lösen: mailsig_02 [randomsig.file] also wahlweise mit oder ohne Argument aufzurufen (s. u.)
jein. Es ging mir darum ein möglichst kleines und schnelle Programm zu haben, das ausserdem einen möglichst keinen Speicherfootprint hat. Deshalb hab ich in das binary nur das reingepackt was ich wirklich brauchte. (Auch wenn auf heutigen Rechnern ein if mehr oder weniger kein Beinbruch ist.)
#else if ((fd=fopen(argv[1],"ro")) == NULL){ #endif
"ro"? - lt. meiner Manualpage gibt es nur "r" für den read-only Modus.
Ouch. Da war "rb" der Vater des Gedanken. Wobei das b unter Linux auch wieder bedeutungslos ist. Interessanterweise funktioniert das bei meinem GCC 3.3.4 ohne Probleme *grübel*
Ab hier habe ich ein grundsätzliches Problem mit Deinem Code.
*Seufz*
Warum liest Du die gesamte Datei in den Speicher (und schlägst Dich mit malloc() und den damit verbundenen Risiken rum), nur um die maximale Anzahl von Signaturen zu ermitteln? Ich halte das für unnötig. Meine Version habe ich unten angehängt.
Es ging mir weniger um die maximale Anzahl von Signaturen als vielmehr darum langsame Plattenzugriffe zu minimieren. D.h. ich lese die Datei nur ein einziges mal von der Platte (Diskcache etc. hab ich nicht berücksichtigt) Wäre aber glaube ich interessant mal einen Vergleich zu haben was schneller ist malloc oder zweimal lesen. (mit diskcache fürchte ich fast; deine Lösung :)
if ((sig[signatur].data = malloc(SIGLEN+1)) == NULL) { perror("malloc"); goto failed; } sig[signatur].data[0]=0x00;
while (fgets(tmp, SIGWIDTH, fd) != NULL) { if (emptyline(tmp) != 0) { strcat(sig[signatur].data,tmp);
Was passiert, wenn eine Zeile länger als SIGWIDTH-1 ist? Dann fehlt Dir das abschliessende \n am Zeilenende und die aktuelle Zeile wird in der Ausgabe an die davor gehende angehängt. Damit ist Deine Prüfung auf max. Zeilenlänge obsolet.
Das ist leider richtig.
Schlimmer: Du prüfst nicht, wie lang der Inhalt von .data bereits ist. Eine Signatur mit mehr als 6 Zeilen a 84 Zeichen würde wahrscheinlich einen Segmentation Fault bewirken, da in nicht initialisierten Speicher geschrieben wird.
Nein, malloc wird vorher immer ausgeführt. Die Signatur fällt später allerdings durchs Raster. [*]
} else { /* Allocate memory for the next signature */ signatur++; if ((sig[signatur].data = malloc(SIGLEN+1)) == NULL) { perror("malloc"); goto failed; } sig[signatur].data[0]=0x00;
Ähm - was machst Du, wenn mehrere leere Zeilen nacheinander kommen? Dann hast Du leere Signaturen.
hmm, doppelte Leerzeilen waren nicht vorgesehen :( Sollte ich wohl noch was einbauen :)
} }
/* if first malloc failes the app probably segfaults later. but may also end without problems so no exit here. */ failed: fclose(fd);
Hm. Ein "Könnte schiefgehen, muss aber nicht* halte ich für nicht so glücklich. Zumindest solltest Du nach einem abgebrochenen malloc() signatur um 1 verringern, damit Du nicht in uninitialiserte Bereiche kommst.
Die Lösung mit signature-- ist gut.
/* get a random number between 1 and signatur */ randnum = rand()%(signatur)+1;
Ja David, da krieg ich bei doppelten Abfragen pro Sekunde die gleiche Signatur. Das sehe ich, da die Signaturdatei bei mir userspezifisch ist, aber nicht als Problem.
/* make sure you don't get flamed :) */ if (strlen(sig[randnum].data) > SIGLEN) {
Wie soll das gehen? Du initialisierst doch .data nur mit SIGLEN+1 und wenn die Signatur länger ist, dann gibts oben einen Buffer Overflow. Also besser gleich oben abfangen.
[*] <- hier ist später Nein, es gibt keinen Overflow. Es wird weiterhin Speicher zugewiesen. Und hier wird nur getestet, ob die Signatur so halbwegs als Signatur durchgeht. Das Problem mit dem \n hast du ja oben schon gefunden.
/* Free the allocated memory */ for (count=1; count <= signatur; count++) { free(sig[count].data); }
Noch ne Kleinigkeit: Warum nutzt Du nicht in Deiner Struktur den Index ab 0? Du würdest Dir eine Menge +1 usw. ersparen :-)
Öhm PEBCAK...
So, nun meine Version. Ich lese die Datei 1,x mal zugunsten einer einfacheren Programmstruktur (und nehme dafür ein paar Millisekunden längere Programmlaufzeit in Kauf). Man kann auch da sicher noch ein paar Dinge optimieren, aber als erster Wurf ;-) Bevorzugen würde ich aber eigentlich immer ein Modell, bei dem das Dateiformat der Signaturdatei bereits beim Erzeugen validiert wird (siehe die anderen Mails im Thread):
Ja, nur hat mich das mit der Validierung gestört. Nach jedem Einfügen einer Signatur eine neue DB erstellen... Na gut, lässt sich auch mit ein bischen bash erledigen und ich denke ich werde wohl in die Richtung gehen. [...]
sigfile = (argc == 1) ? strdup (MYSIGFILE) : strdup (argv[1]); [...]
nett, wenn ich sowas auch mal aus dem FF schreibe, kann ich wohl sagen ich kann ein bischen C ;) Bis das soweit ist poste ich hier halt ab und an ein paar C Schnipsel und lass sie euch zerfleddern. Dann kann ich das auch irgendwann. Greetings Daniel -- Stadtplaner sind Menschen, die Radwege schaffen, indem sie Gehwege halbieren.
Moin Daniel, Am Montag, 11. Oktober 2004 21:53 schrieb Daniel Lord:
On Fri, Oct 08, 2004 at 12:54:05AM +0200, Jan Trippler wrote: [...]
Warum eine Struktur, wenn Du nur ein Array aus *char benötigst?
Das diente nur der besseren Erweiterbarkeit. Eine Struktur benötigt an sich keinen Speicherplatz. Und da ich das Programm irgendwann evtl. noch um Zusatzfunktionen aufstocken wollte... Deshalb struct.
OK, das wollte ich hören ;-) [...]
Warum liest Du die gesamte Datei in den Speicher (und schlägst Dich mit malloc() und den damit verbundenen Risiken rum), nur um die maximale Anzahl von Signaturen zu ermitteln? Ich halte das für unnötig. Meine Version habe ich unten angehängt.
Es ging mir weniger um die maximale Anzahl von Signaturen als vielmehr darum langsame Plattenzugriffe zu minimieren. D.h. ich lese die Datei nur ein einziges mal von der Platte (Diskcache etc. hab ich nicht berücksichtigt) Wäre aber glaube ich interessant mal einen Vergleich zu haben was schneller ist malloc oder zweimal lesen. (mit diskcache fürchte ich fast; deine Lösung :)
Da möchte ich nicht drauf wetten, aber ich behaupte mal, dass es im Bereich bis mehrere 10.000 Einträge nicht sonderlich aufhält (ich lese die Datei ja meist auch nicht 2x bis zum Ende, sondern 1x komplett und dann nur bis zur gesuchten Signatur).
if ((sig[signatur].data = malloc(SIGLEN+1)) == NULL) { perror("malloc"); goto failed; } sig[signatur].data[0]=0x00;
while (fgets(tmp, SIGWIDTH, fd) != NULL) { if (emptyline(tmp) != 0) { strcat(sig[signatur].data,tmp); [...] Schlimmer: Du prüfst nicht, wie lang der Inhalt von .data bereits ist. Eine Signatur mit mehr als 6 Zeilen a 84 Zeichen würde wahrscheinlich einen Segmentation Fault bewirken, da in nicht initialisierten Speicher geschrieben wird.
Nein, malloc wird vorher immer ausgeführt. Die Signatur fällt später allerdings durchs Raster. [*]
Nein, das meine ich nicht. Du initialisierst mit SIGLEN+1, prüfst aber nicht, wieviele Zeilen Du an die aktuelle Signatur (also den .data-Speicher) anhängst. Da könnten 100 Zeilen je 80 Zeichen stehen, was weit über SIGLEN hinausgeht, Du hängst ungeprüft an .data an, wenn dazwischen keine Leerzeile kommt. Damit provozierst Du einen segfault, weil Du über den per malloc reservierten Bereich hinauskommst. Deine Größenprüfungen kommen ja erst nach dem Einlesen der kompletten Datei. [...]
/* make sure you don't get flamed :) */ if (strlen(sig[randnum].data) > SIGLEN) {
Wie soll das gehen? Du initialisierst doch .data nur mit SIGLEN+1 und wenn die Signatur länger ist, dann gibts oben einen Buffer Overflow. Also besser gleich oben abfangen.
[*] <- hier ist später
zu spät (s. o.) ;-)
Nein, es gibt keinen Overflow. Es wird weiterhin Speicher zugewiesen.
Nur eben evtl. zuwenig. [...]
Ja, nur hat mich das mit der Validierung gestört. Nach jedem Einfügen einer Signatur eine neue DB erstellen... Na gut, lässt sich auch mit ein bischen bash erledigen und ich denke ich werde wohl in die Richtung gehen.
Der Vorteil ist der, dass Du wahrscheinlich wesentlich öfter die Datei liest als sie zu ändern. Damit relativiert sich der Aufwand und Du kannst das Programm zur Signaturauswahl (welches wohl wesentlich öfter läuft als die Validierung) einfacher halten. Was dazu kommt: Beim Validieren kannst Du Dir Zeit lassen, dafür geht das Auslesen der Signatur dann deutlich schneller (ich würde mir z. B. in die erste Zeile der validierten Datei die Anzahl der Signaturen schreiben). Jan -- Linux-Quickies: http://www.jan-trippler.de PingoS: http://www.pingos.org TTS-HowTo: https://ssl.pingos.org/pingos/intern/ttshowto/rt.html
Hallo Jan, On Tue, Oct 12, 2004 at 12:46:26AM +0200, Jan Trippler wrote:
Am Montag, 11. Oktober 2004 21:53 schrieb Daniel Lord:
On Fri, Oct 08, 2004 at 12:54:05AM +0200, Jan Trippler wrote:
/me
if ((sig[signatur].data = malloc(SIGLEN+1)) == NULL) { perror("malloc"); goto failed; } sig[signatur].data[0]=0x00;
while (fgets(tmp, SIGWIDTH, fd) != NULL) { if (emptyline(tmp) != 0) { strcat(sig[signatur].data,tmp); [...] Schlimmer: Du prüfst nicht, wie lang der Inhalt von .data bereits ist. Eine Signatur mit mehr als 6 Zeilen a 84 Zeichen würde wahrscheinlich einen Segmentation Fault bewirken, da in nicht initialisierten Speicher geschrieben wird.
Nein, malloc wird vorher immer ausgeführt. Die Signatur fällt später allerdings durchs Raster. [*]
Nein, das meine ich nicht. Du initialisierst mit SIGLEN+1, prüfst aber nicht, wieviele Zeilen Du an die aktuelle Signatur (also den .data-Speicher) anhängst. Da könnten 100 Zeilen je 80 Zeichen stehen, was weit über SIGLEN hinausgeht, Du hängst ungeprüft an .data an, wenn dazwischen keine Leerzeile kommt. Damit provozierst Du einen segfault, weil Du über den per malloc reservierten Bereich hinauskommst. Deine Größenprüfungen kommen ja erst nach dem Einlesen der kompletten Datei.
...jetzt hab ich's auch verstanden/gesehen. Gedacht war das anderst. Implemtiert mal kurz wie oben gesehen. Realloc sollte benutzt werden und immer nur um eine _Zeile_ erweitern. Wie gesagt sollte... also schon wieder PEBCAK so langsam mach ich mir sorgen :( Greetings Daniel, der sich wohl bald ne neue Tastatur kauft *fg* -- No user-serviceable parts inside!
participants (3)
-
Daniel Lord -
David Haller -
Jan.Trippler＠t-online.de