why does this segfault??

Discussion in 'C Programming' started by deejaybags, Sep 11, 2003.

  1. deejaybags

    deejaybags Guest

    could someone please have a look at this and tell me why it segfaults. i
    am confused as all hell!

    stringman.h
    void load_string(char *newString);
    char * remove_string();
    char * remove_upper_string();
    char * remove_lower_string();

    (i havent written the removes yet);

    then the source
    stringman.c
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    /* fundamental structure definition */
    typedef struct list_member{
    char *m_data;
    struct list_member *m_next;
    } MEMBER;

    // initalise head pointer
    MEMBER *headptr;

    void load_string(char *newString)
    {
    printf("checkpoint1");
    //create new list_member for new string
    MEMBER *newmem, *curr;
    printf("checkpoint2");
    if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)
    {
    printf("checkpoint2a");
    fprintf(stderr, "out of memory in new_member\n");
    }
    else
    {
    printf("checkpoint2b");
    /* create memory to copy data into*/
    newmem->m_data = (char *)malloc(strlen(newString)+1);
    printf("checkpoint2c");
    /* copy data into structure, assuming it is a string */
    strcpy(newmem->m_data, newString);
    printf("checkpoint2d");
    /* set the pointer in the structure to null */
    newmem->m_next = (MEMBER *)0;
    }
    printf("checkpoint3");
    // set current to 1st list_member
    curr = headptr;
    printf("checkpoint4");
    // check if list is empty
    if(curr->m_next == (MEMBER *)0)
    {
    printf("checkpoint4a");
    headptr->m_next = newmem;
    }
    else
    {
    // go to the end of the list
    printf("checkpoint4b");
    while(curr->m_next != (MEMBER *)0)
    {
    printf("checkpoint4c");
    curr = curr->m_next;
    }
    printf("checkpoint5");
    // change last ptr to point to current
    curr->m_next = newmem;
    printf("checkpoint6");
    }
    }


    then the testing program
    test.c
    #include "stringman.h"

    int main()
    {
    char *test = "test1", *test2= "test2", *test3 =
    "123456789101112131415161718192021222324252627282930313233343536373839404
    1424344454647484950515253545556575859606162636465666768697071727374757677
    7879808182838485868788899091929394959697989910010110210310410510610710810
    9110111112113114115116117";
    printf("\n test = %s\n",test);
    printf("\n test2 = %s\n",test2);
    printf("\n test3 = %s\n",test3);
    load_string(test);
    printf("load1ok");
    load_string(test2);
    printf("load2ok");
    load_string(test3);
    printf("load3ok");
    printf("all loaded ok so it looks like");
    }


    and then the output


    $ ./test

    test = test1

    test2 = test2

    test3 =
    12345678910111213141516171819202122232425262728293031323334353637383940
    4142434445464748495051525354555657585960616263646566676869707172737475767
    7787980
    8182838485868788899091929394959697989910010110210310410510610710810911011
    1112113
    114115116117
    Segmentation fault (core dumped)

    as you can see, it seams to dump before any of the code in load_string()
    is run.
    im fucking confused as hell. like how can it dump before printing
    checkpoint 1, its the 1st thing in string_load() so how is it dumping
    before that...
     
    deejaybags, Sep 11, 2003
    #1
    1. Advertising

  2. deejaybags

    nrk Guest

    deejaybags wrote:

    > could someone please have a look at this and tell me why it segfaults. i
    > am confused as all hell!
    >
    > stringman.h
    > void load_string(char *newString);
    > char * remove_string();
    > char * remove_upper_string();
    > char * remove_lower_string();
    >
    > (i havent written the removes yet);
    >
    > then the source
    > stringman.c
    > #include <stdio.h>
    > #include <stdlib.h>
    > #include <string.h>
    > /* fundamental structure definition */
    > typedef struct list_member{
    > char *m_data;
    > struct list_member *m_next;
    > } MEMBER;
    >
    > // initalise head pointer
    > MEMBER *headptr;
    >


    headptr is initialized alright. It's initialized to NULL, and
    de-referencing it will produce undefined behavior (one manifestation of
    which can be a "segfault").

    > void load_string(char *newString)
    > {
    > printf("checkpoint1");

    Read your C book carefully. The output stream can be line-buffered or
    fully-buffered. Change this (and all your other printfs) to add a '\n' at
    the end:
    printf("checkpoint1\n");
    If you're paranoid, add a fflush(stdout);. Then you should see atleast some
    of your printf's showing up.

    > //create new list_member for new string
    > MEMBER *newmem, *curr;
    > printf("checkpoint2");
    > if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)

    Style Point: Way too much going on in one line. This style of coding might
    make debugging difficult. Why not write:

    newmem = malloc(sizeof *newmem);
    if ( newmem == NULL )

    Notice how the malloc has been done. This is the clc preferred way. Do
    *NOT* cast the return of malloc. Use the pointer to be allocated to
    determine the size for allocation, if possible.

    > {
    > printf("checkpoint2a");
    > fprintf(stderr, "out of memory in new_member\n");
    > }
    > else
    > {
    > printf("checkpoint2b");
    > /* create memory to copy data into*/
    > newmem->m_data = (char *)malloc(strlen(newString)+1);

    newmem->m_data = malloc(strlen(newString) + 1);

    How about checking the return of that malloc as well? In the worst case,
    atleast put an:
    assert(ptr != NULL);
    after your mallocs.

    > printf("checkpoint2c");
    > /* copy data into structure, assuming it is a string */
    > strcpy(newmem->m_data, newString);
    > printf("checkpoint2d");
    > /* set the pointer in the structure to null */
    > newmem->m_next = (MEMBER *)0;

    What's wrong with:
    newmem->m_next = NULL;

    > }
    > printf("checkpoint3");
    > // set current to 1st list_member
    > curr = headptr;


    Notice that headptr is still NULL and hasn't been initialized to be a valid
    pointer to a MEMBER structure. Perhaps you missed:
    if ( headptr == NULL ) {
    headptr = newmem;
    return;
    }

    That will make a list where the head is simply the first node of the list.

    OTOH, if you wanted a linked list with a dummy head that is always present,
    then you must allocate headptr as in:

    MEMBER dummy;
    MEMBER *headptr = &dummy;

    instead of the declaration you have for headptr now.

    > printf("checkpoint4");
    > // check if list is empty
    > if(curr->m_next == (MEMBER *)0)
    > {
    > printf("checkpoint4a");
    > headptr->m_next = newmem;
    > }
    > else
    > {
    > // go to the end of the list
    > printf("checkpoint4b");
    > while(curr->m_next != (MEMBER *)0)
    > {
    > printf("checkpoint4c");
    > curr = curr->m_next;
    > }
    > printf("checkpoint5");
    > // change last ptr to point to current
    > curr->m_next = newmem;
    > printf("checkpoint6");
    > }
    > }
    >
    >
    > then the testing program
    > test.c
    > #include "stringman.h"
    >
    > int main()
    > {
    > char *test = "test1", *test2= "test2", *test3 =
    > "123456789101112131415161718192021222324252627282930313233343536373839404
    > 1424344454647484950515253545556575859606162636465666768697071727374757677
    > 7879808182838485868788899091929394959697989910010110210310410510610710810
    > 9110111112113114115116117";
    > printf("\n test = %s\n",test);
    > printf("\n test2 = %s\n",test2);
    > printf("\n test3 = %s\n",test3);
    > load_string(test);
    > printf("load1ok");
    > load_string(test2);
    > printf("load2ok");
    > load_string(test3);
    > printf("load3ok");
    > printf("all loaded ok so it looks like");


    return 0;

    Turn up the warning levels in your compiler.

    <OT>If you're using gcc, -Wall -ansi -O atleast.</OT>

    Additional Style Point:
    - Learn to intend your code consistently and in a manner that makes it easy
    to read. If you lack ideas, turn to good C books or look at examples being
    posted by the regulars here.

    HTH,
    nrk.
     
    nrk, Sep 11, 2003
    #2
    1. Advertising

  3. deejaybags

    Artie Gold Guest

    deejaybags wrote:
    > could someone please have a look at this and tell me why it segfaults. i
    > am confused as all hell!


    In general, posting this much code is a Bad Idea. In the future,
    limit your code to the smallest compilable snippet that exhibits the
    problem (which has the added beneficial side effect of often
    pointing you to the error in the process).

    But OK, here goes...

    >
    > stringman.h
    > void load_string(char *newString);
    > char * remove_string();
    > char * remove_upper_string();
    > char * remove_lower_string();
    >
    > (i havent written the removes yet);
    >
    > then the source
    > stringman.c
    > #include <stdio.h>
    > #include <stdlib.h>
    > #include <string.h>
    > /* fundamental structure definition */
    > typedef struct list_member{
    > char *m_data;
    > struct list_member *m_next;
    > } MEMBER;
    >
    > // initalise head pointer
    > MEMBER *headptr;
    >
    > void load_string(char *newString)
    > {
    > printf("checkpoint1");
    > //create new list_member for new string
    > MEMBER *newmem, *curr;
    > printf("checkpoint2");
    > if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)


    Don't cast the return value of malloc(). It's unnecessary and can
    hide other errors (like, for example, failing to include <stdlib.h>).

    Also, it is better to use:
    if ((newmem = malloc( sizeof *newmem) == NULL)


    > {
    > printf("checkpoint2a");
    > fprintf(stderr, "out of memory in new_member\n");
    > }
    > else
    > {
    > printf("checkpoint2b");
    > /* create memory to copy data into*/
    > newmem->m_data = (char *)malloc(strlen(newString)+1);
    > printf("checkpoint2c");
    > /* copy data into structure, assuming it is a string */
    > strcpy(newmem->m_data, newString);
    > printf("checkpoint2d");
    > /* set the pointer in the structure to null */
    > newmem->m_next = (MEMBER *)0;
    > }
    > printf("checkpoint3");
    > // set current to 1st list_member
    > curr = headptr;


    Ding ding ding...
    What's the value of `headptr' here?
    [Hint: you neither initialize it nor assign to it]

    > printf("checkpoint4");
    > // check if list is empty
    > if(curr->m_next == (MEMBER *)0)
    > {
    > printf("checkpoint4a");
    > headptr->m_next = newmem;
    > }
    > else
    > {
    > // go to the end of the list
    > printf("checkpoint4b");
    > while(curr->m_next != (MEMBER *)0)
    > {
    > printf("checkpoint4c");
    > curr = curr->m_next;
    > }
    > printf("checkpoint5");
    > // change last ptr to point to current
    > curr->m_next = newmem;
    > printf("checkpoint6");
    > }
    > }
    >
    >
    > then the testing program
    > test.c
    > #include "stringman.h"
    >
    > int main()
    > {
    > char *test = "test1", *test2= "test2", *test3 =
    > "123456789101112131415161718192021222324252627282930313233343536373839404
    > 1424344454647484950515253545556575859606162636465666768697071727374757677
    > 7879808182838485868788899091929394959697989910010110210310410510610710810
    > 9110111112113114115116117";
    > printf("\n test = %s\n",test);
    > printf("\n test2 = %s\n",test2);
    > printf("\n test3 = %s\n",test3);
    > load_string(test);
    > printf("load1ok");
    > load_string(test2);
    > printf("load2ok");
    > load_string(test3);
    > printf("load3ok");
    > printf("all loaded ok so it looks like");
    > }
    >
    >
    > and then the output
    >
    >
    > $ ./test
    >
    > test = test1
    >
    > test2 = test2
    >
    > test3 =
    > 12345678910111213141516171819202122232425262728293031323334353637383940
    > 4142434445464748495051525354555657585960616263646566676869707172737475767
    > 7787980
    > 8182838485868788899091929394959697989910010110210310410510610710810911011
    > 1112113
    > 114115116117
    > Segmentation fault (core dumped)
    >
    > as you can see, it seams to dump before any of the code in load_string()
    > is run.
    > im fucking confused as hell. like how can it dump before printing
    > checkpoint 1, its the 1st thing in string_load() so how is it dumping
    > before that...


    It isn't. The stream stdout is typically buffered, hence output does
    not appear unless you provide a newline character or explicitly
    flush the stream.

    HTH,
    --ag


    --
    Artie Gold -- Austin, Texas
     
    Artie Gold, Sep 11, 2003
    #3
  4. deejaybags <> wrote:

    >could someone please have a look at this and tell me why it segfaults. i
    >am confused as all hell!
    >
    >stringman.h
    >void load_string(char *newString);
    >char * remove_string();
    >char * remove_upper_string();
    >char * remove_lower_string();
    >
    >(i havent written the removes yet);
    >
    >then the source
    >stringman.c
    >#include <stdio.h>
    >#include <stdlib.h>
    >#include <string.h>
    >/* fundamental structure definition */
    >typedef struct list_member{
    > char *m_data;
    > struct list_member *m_next;
    >} MEMBER;
    >
    >// initalise head pointer
    >MEMBER *headptr;
    >
    >void load_string(char *newString)
    >{
    > printf("checkpoint1");
    > //create new list_member for new string
    > MEMBER *newmem, *curr;
    > printf("checkpoint2");
    > if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)

    Better:
    if ( ( newmem = malloc( sizeof *newmem ) ) == NULL )

    But that's not the problem.

    > {
    > printf("checkpoint2a");
    > fprintf(stderr, "out of memory in new_member\n");
    > }
    > else
    > {
    > printf("checkpoint2b");
    > /* create memory to copy data into*/
    > newmem->m_data = (char *)malloc(strlen(newString)+1);

    Better:
    newmem->m_data = malloc( strlen( newString ) + 1 );

    You failed to check malloc()s return value.
    But that's not the problem.

    > printf("checkpoint2c");
    > /* copy data into structure, assuming it is a string */
    > strcpy(newmem->m_data, newString);
    > printf("checkpoint2d");
    > /* set the pointer in the structure to null */
    > newmem->m_next = (MEMBER *)0;

    Better:
    newmem->m_next = NULL;
    But that's still not the problem.

    > }
    > printf("checkpoint3");
    > // set current to 1st list_member
    > curr = headptr;

    Caaaaarefully...

    > printf("checkpoint4");
    > // check if list is empty
    > if(curr->m_next == (MEMBER *)0)

    Gotcha!!!
    You failed to allocate some memory for headptr (and therefore curr)
    to point to, so curr->m_next invokes dreaded undefined behaviour.
    > {
    > printf("checkpoint4a");
    > headptr->m_next = newmem;
    > }

    Change the above to:

    // check if list is empty
    if( headptr == NULL )
    {
    printf("checkpoint4a");
    headptr = newmem;
    }
    > else
    > {
    > // go to the end of the list
    > printf("checkpoint4b");
    > while(curr->m_next != (MEMBER *)0)
    > {
    > printf("checkpoint4c");
    > curr = curr->m_next;
    > }

    That's a very inefficient way to do it. Maybe you want to maintain a
    pointer to your last element, or just insert at the head of the list.

    > printf("checkpoint5");
    > // change last ptr to point to current
    > curr->m_next = newmem;
    > printf("checkpoint6");
    > }
    >}
    >
    >
    >then the testing program
    >test.c
    >#include "stringman.h"
    >
    >int main()
    >{
    > char *test = "test1", *test2= "test2", *test3 =
    >"123456789101112131415161718192021222324252627282930313233343536373839404
    >1424344454647484950515253545556575859606162636465666768697071727374757677
    >7879808182838485868788899091929394959697989910010110210310410510610710810
    >9110111112113114115116117";
    > printf("\n test = %s\n",test);
    > printf("\n test2 = %s\n",test2);
    > printf("\n test3 = %s\n",test3);
    > load_string(test);
    > printf("load1ok");
    > load_string(test2);
    > printf("load2ok");
    > load_string(test3);
    > printf("load3ok");
    > printf("all loaded ok so it looks like");
    >}
    >
    >
    >and then the output
    >
    >
    >$ ./test
    >
    > test = test1
    >
    > test2 = test2
    >
    > test3 =
    >12345678910111213141516171819202122232425262728293031323334353637383940
    >4142434445464748495051525354555657585960616263646566676869707172737475767
    >7787980
    >8182838485868788899091929394959697989910010110210310410510610710810911011
    >1112113
    >114115116117
    >Segmentation fault (core dumped)
    >
    >as you can see, it seams to dump before any of the code in load_string()
    >is run.

    Right, it /seems/ so, but: if you just had appended a '\n' to the
    strings you print at your checkpoints (or did a fflush(stderr) after
    each printf), you would have noticed that your program runs fine till
    "checkpoint4" is printed, and then segfaults - reason given above.


    >im fucking confused as hell. like how can it dump before printing
    >checkpoint 1, its the 1st thing in string_load() so how is it dumping
    >before that...


    Relax, we've all gone through it. :)


    Some more suggestions:
    - don't use C99 single line comments when posting code here: it may
    result in broken code if a line is wrapped by the newsreader, and
    some people use compilers not capable to deal with //-comments

    - do not cast the return value of malloc: you gain nothing but hiding
    the fact you eventually forgot to #include <stdlib.h>

    - just use NULL insted of (some_type *)0, that's what it is made for

    - it's better style to write

    my_ptr = malloc( sizeof *my_ptr * NUMBER );

    instead of

    my_ptr = malloc( sizeof (my_ptr_type) *NUMBER );

    If the type of my_ptr ever changes in the future, you do not have to
    go through your code and fix all the malloc()s.

    Regards

    Irrwahn
    --
    I can't see it from here, but it looks good to me.
     
    Irrwahn Grausewitz, Sep 11, 2003
    #4
  5. deejaybags

    deejaybags Guest

    You guys are the sikest!!!! thanks heaps *punches self in head* had a
    feeling the headptr was the prob but couldnt see the error with my
    newbie brain! thanks again!! your all the best!!!!!!!!!!

    peace
    eye-bags-won
     
    deejaybags, Sep 12, 2003
    #5
  6. On Thu, 11 Sep 2003 22:40:31 UTC, nrk <> wrote:

    > > if((newmem=(MEMBER *)malloc(sizeof(MEMBER)))==(MEMBER *)0)

    > Style Point: Way too much going on in one line. This style of coding might
    > make debugging difficult. Why not write:


    Not a style point, a point to let the compier give a diagnostic when
    you've forgotten to include stdlib.h. It helps you to debug your code
    as it checks that you have the prototype known.

    Never use a function that returns anything except int without the
    prototype known.

    > newmem = malloc(sizeof *newmem);
    > if ( newmem == NULL )
    >
    > Notice how the malloc has been done. This is the clc preferred way. Do
    > *NOT* cast the return of malloc. Use the pointer to be allocated to
    > determine the size for allocation, if possible.
    >
    > > {
    > > printf("checkpoint2a");
    > > fprintf(stderr, "out of memory in new_member\n");
    > > }
    > > else
    > > {
    > > printf("checkpoint2b");
    > > /* create memory to copy data into*/
    > > newmem->m_data = (char *)malloc(strlen(newString)+1);

    > newmem->m_data = malloc(strlen(newString) + 1);
    >
    > How about checking the return of that malloc as well? In the worst case,
    > atleast put an:
    > assert(ptr != NULL);


    Never use assert to check data. assert is a debug function that will
    either kill your program or even not active in productive code because
    the compiler has the rights to eliminate it when not transating for
    DEBUG is done.
    assert is good to check for programming errors, but never for data
    errors. a fail on lack of memory is mostenly not a cause the program
    has to fail, it has at least to cleanup something (cleanup internal
    data, save something to disk....) instead to break immediately with a
    message the user does know about or interested on.


    --
    Tschau/Bye
    Herbert

    eComStation 1.1 Deutsch Beta ist ver├╝gbar
     
    The Real OS/2 Guy, Sep 13, 2003
    #6
  7. "The Real OS/2 Guy" <> wrote:

    <SNIP>
    >Never use a function that returns anything except int without the
    >prototype known.
    >


    Never use any function without a prototype in scope.

    <SNIP>

    --
    do not write: void main(...)
    do not use gets()
    do not cast the return value of malloc()
    do not fflush( stdin )
    read the c.l.c-faq: http://www.eskimo.com/~scs/C-faq/top.html
     
    Irrwahn Grausewitz, Sep 13, 2003
    #7
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Juho Saarikko
    Replies:
    6
    Views:
    401
    Christophe Cavalaria
    May 18, 2004
  2. Tim Peters
    Replies:
    1
    Views:
    323
    Juho Saarikko
    May 17, 2004
  3. Mr. SweatyFinger

    why why why why why

    Mr. SweatyFinger, Nov 28, 2006, in forum: ASP .Net
    Replies:
    4
    Views:
    912
    Mark Rae
    Dec 21, 2006
  4. Mr. SweatyFinger
    Replies:
    2
    Views:
    2,002
    Smokey Grindel
    Dec 2, 2006
  5. Andrey Vul
    Replies:
    8
    Views:
    687
    Richard Bos
    Jul 30, 2010
Loading...

Share This Page