Grundlagenfrage zu char * und asprintf

  • Hallo an alle C-Experten,


    in der Funktionsbeschreibung von asprintf(&buffer, ...) ist leider nichts davon enthalten, ob ein bereits reservierter Speicherbereich wie hier

    Code
    1. char *timestr = NULL;
    2. asprintf(&timestr, "%s", IndexToHMSF(Current, DisplayFrames));
    3. asprintf(&timestr, "%s / %s ", timestr, IndexToHMSF(Total));
    4. Text(width - cBitmap::Width(timestr) - 14, ty + 3, timestr, clrWhite, clrBackground);
    5. free(timestr);


    in Zeile 2 in der Zeile 3 wieder freigeben wird?


    * Wenn ja warum wird wie hier

    Code
    1. char *Prompt;
    2. asprintf(&Prompt, tr("Press key for '%s'"), tr(cKey::ToString(NewKey)));


    nicht Prompt mit NULL initalisiert?


    oder wofür steht dann die 2 in diesem Code

    Code
    1. static char *buffer = NULL;
    2. free(buffer);
    3. asprintf(&buffer, "%s/plugins%s%s", configDirectory, PluginName ? "/" : "", PluginName ? PluginName : "");


    Alle Beispiele sind dem VDR-Code entnommen.


    Hardy

    Suche: 6 Richtige für die nächste Lottoziehung
    PS: Wer Rechtschreibfehler findet, darf sie behalten!

  • Hallo,


    also wenn ich mich recht erinnere, gibt's im ersten Fall ein Speicherleck.
    Die Initialisierung mit NULL ist meiner Meinung nach nicht notwendig.
    Der free-Aufruf vom dritten Fall ist mir auch unklar, ich denke nicht, dass der was bewirkt.


    Achim

  • Hi,


    Quote

    Original von HFlor
    in der Funktionsbeschreibung von asprintf(&buffer, ...) ist leider nichts davon enthalten, ob ein bereits reservierter Speicherbereich wie hier

    Code
    1. char *timestr = NULL;
    2. asprintf(&timestr, "%s", IndexToHMSF(Current, DisplayFrames));
    3. asprintf(&timestr, "%s / %s ", timestr, IndexToHMSF(Total));
    4. Text(width - cBitmap::Width(timestr) - 14, ty + 3, timestr, clrWhite, clrBackground);
    5. free(timestr);


    in Zeile 2 in der Zeile 3 wieder freigeben wird?


    Wenn nicht noch wichtige Zeilen fehlen, dürfte dort ein klassisches Speicherleak sein, da der Speicher der mit asprinf() zugeteilt wird, auch mit free() wieder freigeben werden muss, sonst ist keine Langzeitstabilität des Programmes gewährleistet.
    Bei selten durchlaufenen Funktionen fällt sowas nicht mal auf, bei der heutigen RAM Bestückung.


    Und da in der man-Page nichts von einer automatischen Freigabe steht, wird dies auch nicht stattfinden.


    Quote

    Original von HFlor
    * Wenn ja warum wird wie hier

    Code
    1. char *Prompt;
    2. asprintf(&Prompt, tr("Press key for '%s'"), tr(cKey::ToString(NewKey)));


    nicht Prompt mit NULL initalisiert?


    Die Initialisierung mit NULL ist im Prinzip nicht notwendig, entspricht aber einem sauberen Programmierstil, keine uninitalisierten Variablen einzusetzen.
    Allerdings ist mit der Kontrolle der Variable auf NULL überprüfbar ob das interne malloc der Funktion fehlgeschlagen ist.
    Zumal mit dem Setzen auf NULL und dem prüfen darauf kann auch eine doppelte und damit sinnlose Freigabe verhindert werden.



    Quote

    Original von HFlor


    oder wofür steht dann die 2 in diesem Code

    Code
    1. static char *buffer = NULL;
    2. free(buffer);
    3. asprintf(&buffer, "%s/plugins%s%s", configDirectory, PluginName ? "/" : "", PluginName ? PluginName : "");


    So jetzt bin ich mit nicht mehr sicher, da ich persönlich keine
    statischen Variablen innerhalb von Funktionen mehr einsetze, weil
    der damit resultierende Code nicht mehr Threadsafe ist,


    Wenn static char * ausserhalb der eigentlich Funktion liegt, erfolgt
    die initialisierung wärend des Startup, bei nachschlagen in der Literatur
    konnte aber auf die Schnelle nichts zu diesen Fall finden ob dies
    auch so ist, ansonsten wäre es technischer Unfug.



    Ich habe auch schon mal versucht valgrind auf den vdr zuhetzen,
    aber der danach vollausgelasstet und es ging nichts mehr, aber in
    showpic von glcd-plugin hat es diesbezüglich wertvolle informationen geliefert.



    HTH,
    Andreas

  • Hallo sundin,


    Quote

    Original von sundin
    ...also wenn ich mich recht erinnere, gibt's im ersten Fall ein Speicherleck...


    Ist aber Orginal aus 'theme.c'


    Quote

    ... Der free-Aufruf vom dritten Fall ist mir auch unklar, ich denke nicht, dass der was bewirkt...


    Es ist als static definiert und läuft in einer Schleife, so das der Aufruf vor free durchaus Sinn macht, wenn es nicht Bestandteil von asprintf ist.


    Hardy

    Suche: 6 Richtige für die nächste Lottoziehung
    PS: Wer Rechtschreibfehler findet, darf sie behalten!

  • Hallo,


    Quote

    Ist aber Orginal aus 'theme.c'


    Deswegen darf's doch trotzdem falsch sein :)


    Quote

    Es ist als static definiert und läuft in einer Schleife, so das der Aufruf vor free durchaus Sinn macht, wenn es nicht Bestandteil von asprintf ist.


    Ups, das static hab ich übersehen. Dann machts vermutlich schon Sinn, obwohl mir das Verhalten beim *ersten* Aufruf bzw. Durchlauf der Schleife nicht klar ist.


    Achim

  • Hallo,


    free(NULL) macht einfach nichts. Also ist:


    ststic char *bla = NULL;
    free(bla);
    asprintf(&bla, ...);


    eine Abkürzung für:


    statis char *bla = NULL;
    if (bla) free(bla);
    asprintf(&bla, ...);


    Die Stelle in theme.c ist tatsächlich falsch, da müsste für die zweite Zeile ein zweites char* genommen werden, und hinterher müssten beide gefree'd werden.

  • Quote

    Die Stelle in theme.c ist tatsächlich falsch, da müsste für die zweite Zeile ein zweites char* genommen werden, und hinterher müssten beide gefree'd werden.


    oder einfach die Zeile


    Code
    1. asprintf(&timestr, "%s / %s ", IndexToHMSF(Current, DisplayFrames), IndexToHMSF(Total));


    geändert werden.


    Edit: Da das ganze in cReplayControlTheme::DrawProgress steht und somit bei eingeschaltetem Statusbalken mindestens jede Sekunde aufgerufen wird, ist es doch ein kritischer Fehler ...


    Hardy

    Suche: 6 Richtige für die nächste Lottoziehung
    PS: Wer Rechtschreibfehler findet, darf sie behalten!

    The post was edited 3 times, last by HFlor ().

  • Ich verstehe zwar nur Bahnhof mit Bratkartoffeln, aber es ist immer wieder schön sowas zu lesen (Fragt mich nicht warum ich sowas lese wenn ich gar keine Ahnung davon habe.)



    Gruss,


    Jörg



    P.S.: Ich sollte wirklich mal wieder damit anfangen. Vielleicht kann ich dann irgendwann mal wieder mitreden ;)

    debian 6.0.7 64-bit, kernel 3.10.0, 2xBudget-CI,Cine S2 V6.5,vdr (2.0.2/2.0.0), vdr-sxfe,remote-plugin + EPSON EH-TW4400 HD Beamer :)

  • Hallo Jörg,


    Quote

    Original von jackfritt
    Ich verstehe zwar nur Bahnhof mit Bratkartoffeln, aber es ist immer wieder schön sowas zu lesen (Fragt mich nicht warum ich sowas lese wenn ich gar keine Ahnung davon habe.)


    Was verstehst Du nicht? Meine C-Kenntnisse waren auch ca. 8 Jahre alt und innerhalb von 2 Monaten habe ich mich wieder eingearbeitet.


    Hardy

    Suche: 6 Richtige für die nächste Lottoziehung
    PS: Wer Rechtschreibfehler findet, darf sie behalten!

  • Meine C- Kentnisse sind 12 Jahre alt. Aber über ein etwas grösseres "Hello" bin ich damals nicht gekommen. Also mach dir keine Sorgen wenn ich hier nix verstehe :)


    Wenn ich mich in das Thema wieder einarbeite und soweit kommen will wie Ihr
    brauch ich noch mindestens 1 Jahr oder mehr.



    Gruss,


    Jörg

    debian 6.0.7 64-bit, kernel 3.10.0, 2xBudget-CI,Cine S2 V6.5,vdr (2.0.2/2.0.0), vdr-sxfe,remote-plugin + EPSON EH-TW4400 HD Beamer :)

  • Hat diese Änderung jemand probiert?

    Code
    1. asprintf(&timestr, "%s / %s ", IndexToHMSF(Current, DisplayFrames), IndexToHMSF(Total));


    Bei mir funktioniert es nämlich nicht.
    Statt der hinteren Totaldauer, wird dann auch die Current angezeigt.


    Ich weiß nicht warum, aber es geht nicht.


    lg, Gerhard

  • Hallo Gerhard,

    Quote

    Original von gestein
    Bei mir funktioniert es nämlich nicht.
    Statt der hinteren Totaldauer, wird dann auch die Current angezeigt.


    Geht bei mir auch nicht. Die Funcktion ' IndexToHMSF' verwendet einen 'static buffer'


    es sind alle Zeilen

    Code
    1. char *timestr = NULL;
    2. asprintf(&timestr, "%s", IndexToHMSF(Current, DisplayFrames));
    3. asprintf(&timestr, "%s / %s ", timestr, IndexToHMSF(Total));
    4. Text(width - cBitmap::Width(timestr) - 14, ty + 3, timestr, clrWhite, clrBackground);
    5. free(timestr);


    durch

    Code
    1. char *timestr1 = NULL;
    2. char *timestr2 = NULL;
    3. asprintf(&timestr1, "%s", IndexToHMSF(Current, DisplayFrames));
    4. asprintf(&timestr2, "%s / %s ", timestr1, IndexToHMSF(Total));
    5. Text(width - cBitmap::Width(timestr2) - 14, ty + 3, timestr2, clrWhite, clrBackground);
    6. free(timestr1);
    7. free(timestr2);


    zu ersetzt werden wie bereits von LordJaxom gesagt wurde.


    Hardy

    Suche: 6 Richtige für die nächste Lottoziehung
    PS: Wer Rechtschreibfehler findet, darf sie behalten!