Herzlich Willkommen, lieber Gast!
  Sie befinden sich hier:

  Forum » C / C++ (ANSI-Standard) » Fehler / schlechtes Coding in Funktion

Forum | Hilfe | Team | Links | Impressum | > Suche < | Mitglieder | Registrieren | Einloggen
  Quicklinks: MSDN-Online || STL || clib Reference Grundlagen || Literatur || E-Books || Zubehör || > F.A.Q. < || Downloads   

Autor Thread - Seiten: > 1 < [ 2 ]
000
21.07.2003, 17:44 Uhr
~xodiak
Gast


Hallo Leute!

Ich habe eine Frage bezüglich einer von mir geschriebenen Funktion.
Kann mir jemand nen Tipp geben, was an dieser Funktion schief laufen könnte? Irgendwann bricht mir mein Programm (nach ungefähr dem 2000. ten Durchlauf mit einem Fehler in malloc.c in Zeile 3049 raus. Ich komm mit dem gdb aber nicht drauf :(
Jeden Speicherbereich den ich mir irgendwo mit malloc hol, geb ich mit free auch wieder (genau einmal!) frei, sobald ich ihn nicht mehr benutze.

Die Funktion getvalue gibt einen char * zurück.
FALSE ist auf 0 und TRUE auf 1 mittels enum definiert.
LINESIZE ist 300, das ist absolut ausreichend.
insertillegal gibt nix zurück und funktioniert auch tadellos. Ein Compilieren
mit -Wall hilft auch nicht (Zeigt nix an). Ich benutze SuSe 8.0 mit gcc 2.95.2 und gdb 4.18.

Dankeschön schonmal fürs durchlesen.


C++:
void check_legal(char *tname, char *dname, float size, int count, int cut)
{

    char *buffer = malloc(LINESIZE), *buffer2 = dname + cut;
    char x = FALSE;
    printf("check_legal %ld\n", ++counter);

    strcpy(buffer, getvalue(tname, "legaldir"));
    if(strcmp(getvalue("global", "legaldir"),getvalue(tname, "legaldir")) != 0)
        sprintf(buffer, "%s;%s", buffer, getvalue("global", "legaldir"));
    buffer = strtok(buffer, ";\r\n");
    if(strncmp(buffer2, "/", 1) == 0) buffer2++;
    while(buffer != NULL && strcmp(buffer2, "") != 0)
    {
        if(strcmp(buffer2, buffer) == 0)
        {
            x = TRUE;
            break;
        }
        buffer = strtok(NULL, ";\r\n");
    }
    if(!x) insert_illegal(size, dname, count);
    free(buffer);
    printf("check_legal Ende\n");
    return;
}



--edit: cpp-tags. Das könntet ihr übrigens ruhig mal selbst machen.

Dieser Post wurde am 21.07.2003 um 17:47 Uhr von 0xdeadbeef editiert.
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
001
21.07.2003, 17:49 Uhr
0xdeadbeef
Gott
(Operator)


Wird in der Funktion getvalue zufällig ein String per malloc hergestellt und zurückgegeben?

Übrigens solltest du dir für solche Dinge mal splint ankucken.
--
Einfachheit ist Voraussetzung für Zuverlässigkeit.
-- Edsger Wybe Dijkstra

Dieser Post wurde am 21.07.2003 um 17:50 Uhr von 0xdeadbeef editiert.
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
002
21.07.2003, 17:56 Uhr
~xodiak
Gast


naja, da wird ein pointer auf ein Element einer Struktur zurückgegeben. Anbei die Funktion. Sorry für das mit den Tags, hab übersehen *schäm*.




C++:
char *getvalue(char *section, char *key)
{
    struct config *p = cbegin;
    while(p)
    {
        if(strcmp(section, p->section) == 0 && strcmp(key, p->key) == 0)
        {
            return p->value;
        }
        p = p->next;
    }
    if (strcmp(section, "global") == 0) return NULL;
    else return getvalue("global", key);
}

 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
003
21.07.2003, 18:02 Uhr
0xdeadbeef
Gott
(Operator)


Du dereferenzierst möglicherweise NULL-Pointer, wenn du sagst:

C++:
if(strcmp(getvalue("global", "legaldir"),getvalue(tname, "legaldir")) != 0)
        sprintf(buffer, "%s;%s", buffer, getvalue("global", "legaldir"));


wenn getvalue NULL zurückgibt, kriegt strcmp NULL-Pointer zugesteckt und dreht wahrscheinlich am Rad. Abgesehen davon ist das nicht performant, du führst dieselbe, zeitaufwändige Funktion zweimal aus. machs besser so:

C++:
char *tmp1, *tmp2;
if( (tmp1 = getvalue("global", "legaldir")) != NULL &&
    (tmp2 = getvalue(tname, "legaldir")) != NULL &&
    strcmp(tmp1, tmp2) != 0) {
        strcat(buffer, ";"); /* per sprintf einen Buffer in sich selbst zu schreiben ist gefährlich. */
        strcat(buffer, tmp1);
    }


--
Einfachheit ist Voraussetzung für Zuverlässigkeit.
-- Edsger Wybe Dijkstra

Dieser Post wurde am 21.07.2003 um 18:09 Uhr von 0xdeadbeef editiert.
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
004
21.07.2003, 18:05 Uhr
~xodiak
Gast


danke für den Tip, werd das gleich mal verifizieren. Ist Dir sonst noch was (Schlechtes/Verbesserungswürdiges) aufgefallen? Bin ein ziemlicher Newbie zu C...
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
005
21.07.2003, 18:12 Uhr
~xodiak
Gast


Nachtrag:

ich habe es jetzt endlich hinbekommen, dass mir der gdb anzeigt, wo er rausbricht. Er stirbt an der Stelle
free(buffer);
Kann mir jemand erklären, warum das so ist?
Ich meine, der Speicherbereich wird zu Beginn der Funktion reserviert und am Ende wieder freigegeben.

Check ich nich
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
006
21.07.2003, 18:20 Uhr
0xdeadbeef
Gott
(Operator)


Ja, da wär noch was. Warum benutzt du malloc? Ich geh doch recht in der Annahme, dass LINESIZE konstant ist, dann kannst du auch einfach schreiben

C++:
char buffer[LINESIZE];


das hat den Vorteil, dass du den Speicher nicht von Hand freigeben musst. buffer ist jetzt eine lokale Variable, die nach dem Ende der Funktion automatisch freigegeben wird. Natürlich geht dann diese Zeile nicht mehr:

C++:
    buffer = strtok(buffer, ";\r\n");


dafür müsstest du halt einen anderen,nach Möglichkeit unbenutzten char* benutzen. tmp würde sich anbieten (siehe unten). Ist aber auch besser, wenn du die Zeile rausnimmst, an dieser Stelle leckst du nämlich Speicher. Ansonsten ist es wichtig, zu beachten, dass strtok ihr erstes Argument verändert. In diesem Fall ist das egal, aber sowas kann zu merkwürdigen Effekten führen. Übrigens kann man die if-Abfrage von eben noch etwas effizienter gestalten:

C++:
char *tmp;
if( (tmp = getvalue("global", "legaldir")) != NULL &&
    buffer != NULL &&
    strcmp(tmp, buffer) != 0) {
        strcat(buffer, ";"); /* per sprintf einen Buffer in sich selbst zu schreiben ist gefährlich. */
        strcat(buffer, tmp1);
    }


Weil in buffer ja schon der Wert von getvalue(tname, "legaldir") drinsteht. Diese Zeile

C++:
if(strncmp(buffer2, "/", 1) == 0) buffer2++;


geht effizienter so:

C++:
if(buffer2[0] == '/') ++buffer2;


allerdings ist das ne ziemlich kleine Sache. Ansonsten finde ich es ein bisschen merkwürdig, dass die Funktion check_legal, die etwas überprüfen soll, keinen Rückgabewert hat, aber ich kenne ja den Rest des Programms nicht.
--
Einfachheit ist Voraussetzung für Zuverlässigkeit.
-- Edsger Wybe Dijkstra
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
007
21.07.2003, 18:22 Uhr
0xdeadbeef
Gott
(Operator)


Wart mal kurz. Schau dir das hier mal an:

C++:
    while(buffer /* hier statt buffer tmp, wenn buffer zum Array wird */ != NULL && strcmp(buffer2, "") != 0)
    {
        if(strcmp(buffer2, buffer) == 0)
        {
            x = TRUE;
            break;
        }
        [b]buffer = strtok(NULL, ";\r\n");[/b] /* was soll das denn? */
    }


Ich bin nicht sicher, was dabei rumkommt, aber gesund sieht es nicht aus. strtok als erstes Argument NULL zu geben, ist im besten Fall selbstmörderisch.
--
Einfachheit ist Voraussetzung für Zuverlässigkeit.
-- Edsger Wybe Dijkstra
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
008
21.07.2003, 18:23 Uhr
~xodiak
Gast


sie muss ja nix zurückgeben
Nein im Ernst, sie führt ja wenn x = 0 einen "insert_illegal" aus.
Das tut dann schon.
Danke für Deine schnelle, kompetente Hilfe. Ich bin schon fleissig am Umstricken.
*wiederwasgelernt,ahaeffektistda*
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
009
21.07.2003, 18:25 Uhr
0xdeadbeef
Gott
(Operator)


Darum gings ja. Ist aber auch keine ganz einfache Sache, die du da vorhast. Sieht mir nach nem ziemlichen Fummelparser aus...
--
Einfachheit ist Voraussetzung für Zuverlässigkeit.
-- Edsger Wybe Dijkstra
 
Profil || Private Message || Suche Download || Zitatantwort || Editieren || Löschen || IP
Seiten: > 1 < [ 2 ]     [ C / C++ (ANSI-Standard) ]  


ThWBoard 2.73 FloSoft-Edition
© by Paul Baecher & Felix Gonschorek (www.thwboard.de)

Anpassungen des Forums
© by Flo-Soft (www.flo-soft.de)

Sie sind Besucher: