Thread: Segmentation fault and can`t find where.

  1. #1
    Registered User
    Join Date
    Sep 2014
    Posts
    121

    Unhappy Segmentation fault and can`t find where.

    Hello, I`ve almost finished the program I am working at ( not commercial, it`s for me ). And I still can`t get what is wrong. I get segmentation fault on 2nd call of specific function. Here is my source code of problematic code, or at least where I think some bad things happen, but really I can`t track them out. With valgrind I was able to find some errors, but please, lend me a hand.
    So the idea - I have 2 lists of strings. One for directories, one for files. A function to walk a directory recursively, and to add to front in both lists respectively for files and for dirs. Then do something on these files. Here is my code for the functions I am using and will explain where I am failing.

    DList.cpp
    Code:
    DList* createList(const char* message) {
        DList* dl =  (DList*)malloc(sizeof(DList));
        dl->head = dl->tail = NULL;
        printf("List created at %p; message:[%s]\n", dl, message);
        if ( dl != NULL ) return dl;
        else return NULL;
    }
    
    void addFront(const char *data, DList* list) {
        LNode *tmp = NULL;
        if ( list->head == NULL ) {
            tmp = (LNode*) malloc(sizeof(LNode));
            tmp->data = (char*) malloc(strlen(data)+1);
           tmp->begin = tmp->data;
            tmp->data = strndup(data, strlen(data));
            *tmp->data++ = '\0';
            tmp->index = _index++;
            tmp->next = list->head ;
            list->head = list->tail = tmp;
        } else {
            tmp = (LNode*)malloc(sizeof(LNode));
            tmp->data = (char*) malloc(strlen(data)+1);
            tmp->begin = tmp->data;
            tmp->data = strndup(data, strlen(data));
            *tmp->data++ = '\0';
            tmp->index = _index++;
            tmp->next = list->head;
            list->head = tmp;
        }
    }
    
    void freeList(DList* list) {
        while ( list->head != NULL ) {
            LNode* removed = list->head;
            list->head = list->head->next;
           free(removed->begin);
            free(removed);
        }
    }
    
    void forEach(DList* list, _doSomething doit) {
        LNode* h = list->head;
        while ( h ) {
            doit(h);
            h = h->next;
        }
    }
    
    void* printFileFromList(LNode *list) {
    
        fprintf(stdout, "[%s][%d]\n", list->data, list->index);
    }
    Dirwalker.cpp
    Code:
    int init_dirWalk(int argc, char** argv) {
        DList* __dirs = createList("Directories");
        DList* __files = createList("Files");
        int i=1;
        while ( i < argc ) {
            walkDir(__dirs, __files, argv[i++], RECURSIVE);
        }
        forEach(__dirs, printFileFromList);
        forEach(__files, printFileFromList);
    
        freeList(__files); //empty lists
        freeList(__dirs);
        free(__files); free(__dirs);
        return 0;
    }
    
    int isDir(const char *fname) {
        struct stat s;
        stat(fname, &s);
        return S_ISDIR(s.st_mode);
    }
    
    void walkDir(DList *dlist, DList* flist, char *basedir, int recursive) {
        DIR* dir;
        struct dirent* ent;
        dir = opendir(basedir);
        if ( dir != NULL ) {
            while ( ent = readdir(dir ) ) {
                if ( (strcmp(ent->d_name, ".") == 0)
                     ||
                     (strcmp(ent->d_name, ".."))==0 ) {
                    continue;
                } 
                char entpath[1024] = "";
                sprintf(entpath, "%s%s\0", basedir, ent->d_name);
                if ( isDir(entpath) ) {
                    if ( recursive > 0) {
                      sprintf(entpath, "%s/\0", entpath);
                      addFront(entpath, dlist);
                      walkDir(dlist, flist, entpath, RECURSIVE);
                    }
                } else {
                        addFront(entpath, flist);
                } 
            } //end while
            closedir(dir);
        } else {
            fprintf(stderr, "\nFailed to walk directory \%s \n", basedir);
            if ( DEBUG ) {
                perror("opendir()");
            }
        } 
    }
    And finally a simple main.c testing
    Code:
    int main(int argc, char** argv) {
    int c;
        while ( (c=getchar()) != EOF ) {
            getchar();
            switch ( c ) {
            case 'p':
            case 'P':
                init_dirWalk(argc, argv); break;
            default: printf("Press P/p to print\n"); break;
            }
        }
    }
    What fails? Well after the first press of p/P, it lists and prints the dir/filse. On second press it gives me a segphault. Really no idea what I got messed up. I`ve listed all the functions I am using so I guess, they should be enough for experts to tell where I am failling.
    Last edited by heatblazer; 09-21-2014 at 12:39 PM.

  2. #2
    Registered User
    Join Date
    Sep 2014
    Posts
    121
    OK, somehow it works, in my example here, I`ve removed a problematic, obviously, call to sprintf in init_dirWalker().... Why sprintf makes this problem - it`s beyound me. The code is:
    Code:
    char buff[1024] = "";
    sprintf(buff, "Called init_dirWalker() \n");

  3. #3
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    27,776
    Is there any special reason that you have variable names like __dirs? I note:
    Quote Originally Posted by C99 Clause 7.1.3 Paragraph 1 (part)
    All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.
    That is, those identifiers are reserved to the implementation, so you should not be naming variables with such names unless you are writing a compiler or standard library implementation.

    This looks strange:
    Code:
    tmp->data = (char*) malloc(strlen(data)+1);
    tmp->begin = tmp->data;
    tmp->data = strndup(data, strlen(data));
    *tmp->data++ = '\0';
    I would have expected:
    Code:
    size_t data_len = strlen(data);
    tmp->begin = malloc(data_len + 1);
    tmp->data = strndup(data, data_len);
    It seems confusing to first assign the result of malloc to tmp->data, then get tmp->begin to point to the same thing, then overwrite tmp->data. Furthermore, a quick check shows that strndup null terminates, so there is no need to do so yourself, and in fact your attempt to null terminate looks wrong since it is equivalent to:
    Code:
    *tmp->data = '\0';
    tmp->data++;
    which basically makes the whole strndup thing pointless and then makes it harder for you to correctly call free on tmp->data.

    Quote Originally Posted by heatblazer
    Why sprintf makes this problem - it`s beyound me.
    This is one of the symptoms of undefined behaviour.
    Quote Originally Posted by Bjarne Stroustrup (2000-10-14)
    I get maybe two dozen requests for help with some sort of programming or design problem every day. Most have more sense than to send me hundreds of lines of code. If they do, I ask them to find the smallest example that exhibits the problem and send me that. Mostly, they then find the error themselves. "Finding the smallest program that demonstrates the error" is a powerful debugging tool.
    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  4. #4
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,056
    Additionally, renaming all the .cpp files to be .c, then removing all the casts on malloc calls would be a good idea.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  5. #5
    Registered User
    Join Date
    Sep 2014
    Posts
    121
    Quote Originally Posted by Salem View Post
    Additionally, renaming all the .cpp files to be .c, then removing all the casts on malloc calls would be a good idea.
    Thank you Salem, I am using qtcreator and that`s why they are cpp.

  6. #6
    Registered User
    Join Date
    Sep 2014
    Posts
    121
    Quote Originally Posted by laserlight View Post
    Is there any special reason that you have variable names like __dirs? I note:

    That is, those identifiers are reserved to the implementation, so you should not be naming variables with such names unless you are writing a compiler or standard library implementation.

    This looks strange:
    Code:
    tmp->data = (char*) malloc(strlen(data)+1);
    tmp->begin = tmp->data;
    tmp->data = strndup(data, strlen(data));
    *tmp->data++ = '\0';
    I would have expected:
    Code:
    size_t data_len = strlen(data);
    tmp->begin = malloc(data_len + 1);
    tmp->data = strndup(data, data_len);
    It seems confusing to first assign the result of malloc to tmp->data, then get tmp->begin to point to the same thing, then overwrite tmp->data. Furthermore, a quick check shows that strndup null terminates, so there is no need to do so yourself, and in fact your attempt to null terminate looks wrong since it is equivalent to:
    Code:
    *tmp->data = '\0';
    tmp->data++;
    which basically makes the whole strndup thing pointless and then makes it harder for you to correctly call free on tmp->data.


    This is one of the symptoms of undefined behaviour.
    Hello, Laserlight. I know your suggestions. Originally it was that code. But this code:
    Code:
    tmp->data = (char*) malloc(strlen(data)+1);
    tmp->begin = tmp->data;
    tmp->data = strndup(data, strlen(data));
    *tmp->data++ = '\0';
    And specially the tmp->begin var, when I try iterate and empty the list, a call to free(tmp->data) gives me segphault, where a call to free(tmp->begin) gives me no errors. I really have no idea, it may be because qt creator uses G++ but I am sure a GCC was set in the options so technically there should be no problems. About the null terminate I`ve entered, it`s because I`ve tried other methods than the strndup, I`ve tried with strcpy and a custom copy function. You are right, the null termination is useless. What bothers me is why after the removal of sprintf() the program no longer reported segfaults?

  7. #7
    Registered User
    Join Date
    Dec 2007
    Posts
    2,673
    Quote Originally Posted by heatblazer View Post
    What bothers me is why after the removal of sprintf() the program no longer reported segfaults?
    Quote Originally Posted by laserlight
    This is one of the symptoms of undefined behaviour.
    What laserlight said.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Segmentation Fault, could not find after debugging
    By Campocalypse in forum C Programming
    Replies: 9
    Last Post: 04-15-2013, 11:53 PM
  2. Segmentation fault, cannot find problem
    By navitude in forum C Programming
    Replies: 4
    Last Post: 03-02-2013, 01:57 PM
  3. Segmentation fault - can't find it
    By überfuzz in forum C++ Programming
    Replies: 14
    Last Post: 11-29-2012, 11:40 PM
  4. Segmentation fault using recursion to find factorial
    By kapok in forum C++ Programming
    Replies: 4
    Last Post: 02-23-2009, 11:10 AM
  5. Can't find the cause of a segmentation fault
    By Decrypt in forum C++ Programming
    Replies: 4
    Last Post: 02-06-2006, 08:28 PM

Tags for this Thread