Memory corruption on freeing a pointer to pointer

Discussion in 'C Programming' started by Sharwan Joram, Aug 23, 2013.

  1. Hi,
    I have a strange problem here in my code. I'am getting memory corruption while trying to free the first element of list. Code snippet and GDB debugging trace below :

    ---------------------------------- code
    char **parameters;
    int idx;
    int parametercount;
    char *saved_token, token;

    parameters = (char **)malloc(parametercount * sizeof(char *)); // Don't use *parameters it breaks.
    for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    parameters[parametercount] = (char *)malloc(30 * sizeof (char *));
    memset(parameters[parametercount], '\0', 30);
    memcpy(parameters[parametercount], token, strlen(token));
    parameters[parametercount] = token;
    }

    /* idx contains the number of tokens */
    if (parameters != NULL){
    for (parametercount = idx; parametercount >= 0; ++parametercount)
    free(parameters[parametercount]);
    free(parameters);
    }

    ---------- Debugging session trace ----

    160 memcpy(temp_token, saved_token, strlen(saved_token));
    (gdb)
    161 idx = parametercount = detect_delim_count(temp_token, delimiters);
    (gdb)
    162 if (temp_token)
    (gdb)
    163 free(temp_token);
    (gdb) n
    171 parameters = (char **)malloc(parametercount * sizeof(char *)); // Don't use *parameters it breaks.
    (gdb)
    172 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb)
    173 parameters[parametercount] = (char *)malloc(30 * sizeof (char *));
    (gdb) p parameters
    $1 = (char **) 0xb6e0f828
    (gdb) n
    174 memset(parameters[parametercount], '\0', 30);
    (gdb)
    175 memcpy(parameters[parametercount], token, strlen(token));
    (gdb)
    172 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb)
    173 parameters[parametercount] = (char *)malloc(30 * sizeof (char *));
    (gdb) p parameters
    $2 = (char **) 0xb6e0f828
    (gdb) n
    174 memset(parameters[parametercount], '\0', 30);
    (gdb) p parameters[parametercount]
    $3 = 0xb6e0f8b8 ""
    (gdb) n
    175 memcpy(parameters[parametercount], token, strlen(token));
    (gdb)
    172 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb) p parameters[parametercount]
    $4 = 0xb6e0f8b8 "param2"
    (gdb) n
    173 parameters[parametercount] = (char *)malloc(30 * sizeof (char *));
    (gdb)
    174 memset(parameters[parametercount], '\0', 30);
    (gdb) p parameters[parametercount]
    $5 = 0xb6e0f938 ""
    (gdb)
    $6 = 0xb6e0f938 ""
    (gdb) n
    175 memcpy(parameters[parametercount], token, strlen(token));
    (gdb) p parameters[parametercount]
    $7 = 0xb6e0f938 ""
    (gdb) n
    172 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb) p parameters[parametercount]
    $8 = 0xb6e0f938 "param3"
    (gdb) n
    173 parameters[parametercount] = (char *)malloc(30 * sizeof (char *));
    (gdb)
    174 memset(parameters[parametercount], '\0', 30);
    (gdb)
    175 memcpy(parameters[parametercount], token, strlen(token));
    (gdb)
    172 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb) p parameters[parametercount]
    $9 = 0xb6e0f9b8 "param4"
    (gdb) n
    201 shutdown = (currentcommand->command_handler)(client_fd, parameters, parametercount);
    (gdb)
    211 executedcommand = currentcommand;
    (gdb)
    216 currentcommand = currentcommand->next;
    (gdb)
    127 while((executedcommand == NULL) && (currentcommand != NULL)){
    (gdb)
    221 if (parameters != NULL){
    (gdb)
    222 for (parametercount = idx; parametercount >= 0; ++parametercount)
    (gdb) p parameters
    $10 = (char **) 0xb6e0f828
    (gdb) p parameters[parametercount]
    $11 = 0x61726170 <Address 0x61726170 out of bounds>
    (gdb) n
    223 free(parameters[parametercount]);
    (gdb) p parameters[parametercount]
    $12 = 0xb6e0f9b8 "param4"
    (gdb) n
    *** glibc detected *** /home/sources/opennop-daemon/opennopd/opennopd: corrupted double-linked list: 0xb6e0f820 ***
    ======= Backtrace: =========
    /lib/libc.so.6[0x4441a1d1]
    /lib/libc.so.6[0x4441a7bd]
    /home/sources/opennop-daemon/opennopd/opennopd[0x805294c]
    /home/sources/opennop-daemon/opennopd/opennopd[0x80520c5]
    /lib/libpthread.so.0[0x4455fadf]
    /lib/libc.so.6(clone+0x5e)[0x4449944e]
    ======= Memory map: ========
    08048000-08056000 r-xp 00000000 fd:01 397476 /home/sources/opennop-daemon/opennopd/opennopd
    08056000-08057000 rw-p 0000d000 fd:01 397476 /home/sources/opennop-daemon/opennopd/opennopd
    08057000-082ce000 rw-p 00000000 00:00 0 [heap]
    44382000-443a1000 r-xp 00000000 fd:01 148916 /usr/lib/ld-2.15.so
    443a1000-443a2000 r--p 0001e000 fd:01 148916 /usr/lib/ld-2.15.so
    443a2000-443a3000 rw-p 0001f000 fd:01 148916 /usr/lib/ld-2.15.so
    443a5000-44550000 r-xp 00000000 fd:01 148917 /usr/lib/libc-2.15.so
    44550000-44551000 ---p 001ab000 fd:01 148917 /usr/lib/libc-2.15.so
    44551000-44553000 r--p 001ab000 fd:01 148917 /usr/lib/libc-2.15.so
    44553000-44554000 rw-p 001ad000 fd:01 148917 /usr/lib/libc-2.15.so
    44554000-44557000 rw-p 00000000 00:00 0
    44559000-4456f000 r-xp 00000000 fd:01 148918 /usr/lib/libpthread-2.15.so
    4456f000-44570000 r--p 00015000 fd:01 148918 /usr/lib/libpthread-2.15.so
    44570000-44571000 rw-p 00016000 fd:01 148918 /usr/lib/libpthread-2.15.so
    44571000-44573000 rw-p 00000000 00:00 0
    44575000-44578000 r-xp 00000000 fd:01 148924 /usr/lib/libdl-2.15.so
    44578000-44579000 r--p 00002000 fd:01 148924 /usr/lib/libdl-2.15.so
    44579000-4457a000 rw-p 00003000 fd:01 148924 /usr/lib/libdl-2.15.so
    448f5000-44911000 r-xp 00000000 fd:01 148934 /usr/lib/libgcc_s-4.7.2-20120921.so.1
    44911000-44912000 rw-p 0001b000 fd:01 148934 /usr/lib/libgcc_s-4.7.2-20120921.so.1
    45642000-45692000 r-xp 00000000 fd:01 148937 /usr/lib/libfreebl3.so
    45692000-45693000 rw-p 00050000 fd:01 148937 /usr/lib/libfreebl3.so
    45693000-45697000 rw-p 00000000 00:00 0
    456a8000-456b0000 r-xp 00000000 fd:01 148938 /usr/lib/libcrypt-2.15.so
    456b0000-456b1000 r--p 00007000 fd:01 148938 /usr/lib/libcrypt-2.15.so
    456b1000-456b2000 rw-p 00008000 fd:01 148938 /usr/lib/libcrypt-2.15.so
    456b2000-456d9000 rw-p 00000000 00:00 0
    b21ff000-b2200000 ---p 00000000 00:00 0
    b2200000-b2a00000 rw-p 00000000 00:00 0 [stack:846]
    b2a00000-b2b00000 rw-p 00000000 00:00 0
    b2bf9000-b2bfa000 ---p 00000000 00:00 0
    b2bfa000-b33fa000 rw-p 00000000 00:00 0 [stack:844]
    b33fa000-b33fb000 ---p 00000000 00:00 0
    b33fb000-b3bfb000 rw-p 00000000 00:00 0 [stack:843]
    b3bfb000-b3bfc000 ---p 00000000 00:00 0
    b3bfc000-b43fc000 rw-p 00000000 00:00 0 [stack:842]
    b43fc000-b43fd000 ---p 00000000 00:00 0
    b43fd000-b4bfd000 rw-p 00000000 00:00 0 [stack:840]
    b4bfd000-b4bfe000 ---p 00000000 00:00 0
    b4bfe000-b53fe000 rw-p 00000000 00:00 0 [stack:836]
    b53fe000-b53ff000 ---p 00000000 00:00 0
    b53ff000-b5bff000 rw-p 00000000 00:00 0 [stack:835]
    b5bff000-b5c00000 ---p 00000000 00:00 0
    b5c00000-b6400000 rw-p 00000000 00:00 0 [stack:834]
    b6400000-b6500000 rw-p 00000000 00:00 0
    b65ff000-b6600000 ---p 00000000 00:00 0
    b6600000-b6e00000 rw-p 00000000 00:00 0 [stack:833]
    b6e00000-b6e2a000 rw-p 00000000 00:00 0
    b6e2a000-b6f00000 ---p 00000000 00:00 0
    b6fd2000-b6fd3000 ---p 00000000 00:00 0
    b6fd3000-b77d3000 rw-p 00000000 00:00 0 [stack:832]
    b77d3000-b77d4000 ---p 00000000 00:00 0
    b77d4000-b7fd6000 rw-p 00000000 00:00 0 [stack:831]
    b7fd6000-b7fda000 r-xp 00000000 fd:01 165010 /usr/lib/libmnl.so.0.1.0
    b7fda000-b7fdb000 r--p 00003000 fd:01 165010 /usr/lib/libmnl.so.0.1.0
    b7fdb000-b7fdc000 rw-p 00004000 fd:01 165010 /usr/lib/libmnl.so.0.1.0
    b7fdc000-b7fe2000 r-xp 00000000 fd:01 165001 /usr/lib/libnfnetlink.so.0.2.0
    b7fe2000-b7fe3000 r--p 00005000 fd:01 165001 /usr/lib/libnfnetlink.so.0.2.0
    b7fe3000-b7fe4000 rw-p 00006000 fd:01 165001 /usr/lib/libnfnetlink.so.0.2.0
    b7fe4000-b7fe5000 rw-p 00000000 00:00 0
    b7fe5000-b7feb000 r-xp 00000000 fd:01 148876 /usr/lib/libnetfilter_queue.so.1.3.0
    b7feb000-b7fec000 r--p 00005000 fd:01 148876 /usr/lib/libnetfilter_queue.so.1.3.0
    b7fec000-b7fed000 rw-p 00006000 fd:01 148876 /usr/lib/libnetfilter_queue.so.1.3.0
    b7ffc000-b7fff000 rw-p 00000000 00:00 0
    b7fff000-b8000000 r-xp 00000000 00:00 0 [vdso]
    bffdf000-c0000000 rw-p 00000000 00:00 0 [stack]

    I am unable to understand the reason of this behaviour.

    --Regards,
    Sharwan Joram
     
    Sharwan Joram, Aug 23, 2013
    #1
    1. Advertisements

  2. Sharwan Joram

    James Kuyper Guest

    Using (char**) is unnecessary; and if this code gets compiled using C90,
    could mask a defect.

    sizeof *parameters should be exactly equivalent to sizeof(char *),
    except for being safer in the even that the type of 'paremeters' ever
    gets changed.. If you've seen something break when you used *parameters,
    that needs investigation.

    You make no attempt, neither above nor below, to confirm whether your
    calls to malloc() were successful. They might not have been. If any
    malloc() call failed, the fact that your code tries to write to the
    allocated memory would render the behavior of your program undefined.
    Memory corruption is a very plausible result if that happens.
    It's simpler, and on some systems, faster, to call calloc() rather than
    malloc() followed by memset().
    Why not just strcpy()?
    Are you certain that strlen(token)<31? If not, the memcpy() will
    overwrite memory beyond that which is allocated, rendering the behavior
    of your program undefined.

    Will your code ever change the parameter strings? If not, you might save
    some memory (and remove the need to check whether strlen(token) > 31) by
    always allocating strlen(token)+1 bytes, and you can drop the call to
    memset(). However, if you do this, you need to make sure that everything
    done with these parameter strings stops at the terminating null
    character, rather than just assuming there will always be 30 safely
    accessible bytes.
    That's the cause of your problem. I can't figure you why you wrote that
    code - but it almost certainly doesn't do what what you think it does.

    You've just replaced the only pointer you have to memory that was
    allocated by malloc(), with a pointer to memory within the array pointed
    at by saved_token. Therefore, you will never be able to free that
    allocated memory, which creates a memory leak. Also, you'll never again
    be able to access the string that you copied into that memory.
    This is where the defect above actually causes your program to fail.
    Since parameters[parametercount] no longer contains a pointer to memory
    that was allocated by malloc(), passing it's value to free() means that
    the behavior of your program is undefined. Memory corruption is a very
    likely consequences of doing something like that.
     
    James Kuyper, Aug 23, 2013
    #2
    1. Advertisements

  3. Hi James,
    One error in the snippet above : This line is commented
    parameters[parametercount] = token;

    1. The parameters never go beyond 30 characters.

    As the aim of the code above is to quickly check the functionality so didn't care the suggestions you mentioned above.

    --Sharwan
     
    Sharwan Joram, Aug 23, 2013
    #3
  4. Sharwan Joram

    Martin Shobe Guest

    What do you think it breaks?
    I'll assume you left out the code that initialized saved_token.
    If you're going to use strlen(token), why not go ahead and use strcpy?
    One of your main problems is here. token is a pointer into the memory
    pointed to by saved_token. After the first call to strtok(), it's not
    going to be a pointer you can pass to free(). Even the first call to
    strtok() might not result in a pointer that can be passed to free().
    More importantly, it overwrites the value already there, leaking that
    memory.
    The for loop is the other main problem. If your comment is correct, then
    idx indexes to first free slot in the parameters array. Since you don't
    appear to have initialized that slot with anything, the call to free is
    problematic. On top of that, you increment parametercount, so the second
    time through (if it gets that far) idx points to the second free slot in
    parameters. This will continue until the undefined behavor from
    attempting to derefence past the end of the array, attempting to free
    garbage, or overflowing idx puts an end to your process. From the
    message below, it appears that attempting to free garbage is what was
    finally caught.

    [snip]

    Martin Shobe
     
    Martin Shobe, Aug 23, 2013
    #4
  5. Sharwan Joram

    James Kuyper Guest

    It's not commented out in the code you displayed. If it is commented out
    in the code that you're actually running, you just wasted a fair amount
    of both my time and yours by not showing me the actual code that failed.

    It's generally a bad idea, when asking for help identifying a code
    defect, to show the person who's helping you anything other than a
    complete program that will, as shown, demonstrate the defect. By
    definition, you don't know what the defect is, or you wouldn't need
    help. It follows that you don't know which part of the program actually
    causes the defect. Anything less than a complete program may be a waste
    of time.
    You may think that your complete program is too big to show me - and
    you're probably right. Which means that what you should do is cut the
    program down to be as small as possible, while still demonstrating the
    problem. Remove one piece at a time (preferably a really big piece, such
    as every line of code that was supposed to execute after the line where
    something failed), and check to see if the problem still occurs. If it
    doesn't, put that piece back in, and choose a different piece to remove.
    If you can't find any remaining pieces that can be removed, that's when
    the program is ready for you to ask for someone's help.
    Note: it is very common for people who are following this process, to
    figure out what the problem is, without even having to ask for help.

    Well, one of the short-cuts you took for in order "to quickly check the
    functionality" may be the cause of the problem you're seeing. Those
    shortcuts are easy, simple things to fix, and I'd recommend doing so
    before doing anything else to investigate this problem.
     
    James Kuyper, Aug 23, 2013
    #5
  6. Sharwan Joram

    Ike Naar Guest

    What is the value of parametercount?
     
    Ike Naar, Aug 23, 2013
    #6
  7. I was going to post that, and then I saw that the debug output showed
    that code is executed between the declaration and the use. The OP has
    not made the code being executed at all clear.
     
    Ben Bacarisse, Aug 23, 2013
    #7
  8. The value of parametercount is indeterminant. The next block of code
    implies it is not known at this time.
    What does saved_token point to? Under what condition can token be
    non-NULL and *token be '\0'?
    This allocates too much memory.
    What purpose does this serve since you immediately replace the zeroed
    out values in the next statement?
    Why not use strcpy and save at least one function call?
    You have now caused a memory leak. parameters[parametercount] used to
    point to the allocated memory. Now it point to somewhere inside
    saved_token.
    If parameters is NULL, all the previous code invokes undefined
    behavior. Essentially, this is if statement must evaluate to true.
    If idx is positive, this loop will never terminate.
    Since parameters[parametercount] no longer contains a value returned
    by malloc, this call to free invokes undefined behavior and frequently
    causes programs to crash.
    This trace does not match the code you have shown us.
    An assignment statement should have been traced here.
    <snip>

    Show us your real code.
     
    Barry Schwarz, Aug 23, 2013
    #8
  9. Sharwan Joram

    James Kuyper Guest

    ....

    It's clear that Sharwan only presented us with part of his program, and
    as I found out the hard way, even some of the lines he did give us
    weren't actually part of the program.
    That's probably correct (though it might be the case that some of the
    parameters are large enough to need that much space). However, your
    comment isn't very helpful without further explanation.

    Sharwan: what Barry is referring to is the fact that you wrote
    30*sizeof(char*), when context suggests that you should have written
    30*sizeof(char), or better yet, 30*sizeof **parameters. sizeof(char*) is
    almost certainly larger than sizeof(char); on many modern systems it
    will be 4; other likely values include 2 and 8.
    Assuming that the above line is more "real" than the

    parameters[parametercount] = token;

    turned out to be, this looks like the best candidate for the main problem.
    It will access memory beyond the end of the parameters array, interpret
    uninitialized memory or memory that's serving some other purpose as if
    it contained a pointer value, and pass that value to free(). That will
    render the behavior of the code undefined for at least two different
    reasons (three if char* can have trap values) almost simultaneously.

    It probably should have been written:

    for (parametercount = idx-1; parametercount >= 0; --parametercount)

    Alternatively, it would have worked just as well going through the array
    in the opposite direction, with less danger of getting the loop
    conditions wrong:

    for(parametercount = 0; parametercount < idx; ++parametercount)
     
    James Kuyper, Aug 23, 2013
    #9
  10. Sharwan Joram

    gdotone Guest

    this is Sharwan Joram code with comments and questions from me as i try to learn C
    this is only a partial bit of the code

    thanks for helpng me learn
    char **parameter; /* parameter is a pointer that points to a character pointer */

    int idx;

    int parameterCount; /* the number of parameters counted */

    char *saved_token, token;

    /*

    get sizeof char pointer
    allocate a memory block the size of parameterCount times the sizeof character pointer
    so, an array is being created, cast the array of memory to be a char **, a pointer to a
    pointer of type char.

    parameterCount has not been initialized.

    */

    parameters = (char **) malloc( parameterCount * sizeof( char * ) );

    /*
    test to see if malloc was able to find memory space.
    */

    if ( parameter != NULL )
    {

    /*
    loop until token && *token (what! token is a char, so what is *token)
    parameter is initialized to 0
    token is initialized to whatever *saved_token first token is.
    ( but *saved_token is not initialized at this point )
    ++parameterCount is increment by 1
    token is assigned the value of next token from the *save_token pointer
    ( but token is declared as a char, strtok() returns a pointer to a char )
    ( so, maybe token should be declared as a char *, a pointer to a char )
    */

    for( parameterCount = 0, token = strtok( saved_token, " ";
    token && *token;
    ++parameterCount, token = strtok( NULL," ") );
    {

    /*
    question: Do you want the size a char or do you want the size of the pointer
    to type char? they may not be the same size, right?

    find the sizeof a char pointer
    multiply that value by 30 and calloc malloc to get a block of memory the size
    of 30 character pointers
    cast that block to be of type char pointer (char *)
    have parameter[parameterCount] point to that block of memory.
    */

    parameters[parametercount] = (char *)malloc(30 * sizeof (char *));


    /*
    void *memset( void *s, int c, size_t n );
    copies c ( converted to unsigned char ) into the first n characters of
    the object pointed to by s. A pointer to the result is returned.
    (referenced from a textbook)

    parameters[ parameterCount ] is being treated as array of pointers to an array
    of chars?
    so, in the 30 position of the array of characters place a '\0' character

    */

    memset(parameters[parameterCount], '\0', 30);

    ....
     
    gdotone, Aug 24, 2013
    #10
  11. Often when you get an error on calling free, the problem is that there's
    been memory corruption in the preceding or following block, which could be in
    a very different part of the program in code space.

    But you are calling strtok() on a wild pointer. That will rewrite some random point in memory with a character zero. If that's not your problem, you're allocating a wild amount of parameters.
    Assuming you've just posted a fragement and these variables are correct, your
    loop condition stops when strtok() returns NULL. Tha't asking for trouble, as you might have more than parametercount tokens in the string. You also say
    that allocating *parameters (* *parametercount?) breaks. That's a bit worrying,
    it shouldn't, nad indicates something wrong somewhere.

    Then as James pointed out, you're setting the string to a constant. You say
    this is commented out. Are you sure?
     
    Malcolm McLean, Aug 24, 2013
    #11
  12. Two things: you say that parameterCount has not been initialised, but
    has it been set in the "real" code bu this time? If not what do you
    expect this like to do? Without having set parameterCount, you have no
    idea.

    Second thing: please switch to this pattern:

    parameters = malloc( parameterCount * sizeof *parameters );

    It's shorter, and you don't have to check any types. Provided the type
    of "parameters" is correct (and it looks fine to me) it is guaranteed to
    allocate the right space for parameterCount objects.
    You have the wrong type for token. It, too, should be a char *.
    No it isn't. Did you mean parameterCount? You need to take a lot of
    care in programming. Little details make all the difference.
    Do you mean this? If *saved_token is uninitialised (and I assume the
    same is true of saved_token) you are no longer in control of what
    happens. You must only operate with data you've set.
    I there some prize for cramming as much as possible into one for
    statement? You have at least one syntax error there, and please note
    that last semi-colon. Do you know what effect it has?

    I would write this in the following way:

    char *token = strtok(saved_token, " ");
    int p_count = 0;
    while (token && *token) {

    /* do the processing of the token */

    token = strtok(NULL, " ");
    p_count += 1;
    }

    Note that (a) I've broken it up intosimpler pieces; (b) I've moved some
    of the declarations closer to the point of use; and (c) I've not re-used
    paramerterCount. It's quite likely that whatever value paramerterCount
    had before will be useful again so lets use a new int for this loop.

    In fact, I think you want something called "maxParameterCount" in the
    original allocation, in which case using paramerterCount here is fine.
    The point is that the number plays different roles in the two places and
    that deserves having two variables.
    If you write

    parameters[parametercount] = malloc(30 * sizeof *parameters[parametercount]);

    you know you are getting space for 30 of the things that can be pointed
    to by parameters[X]. There are, as it happens, char objects, so your
    sizeof expression above is wrong.
    The header files are there for a purpose! It's rarely a good idea to
    manually declare C library functions like this.
    No. parameters[parameterCount] is just a pointer to a character.
    I suggest you try to write a cut-down version of this program and post
    it whole.
     
    Ben Bacarisse, Aug 24, 2013
    #12
  13. But it's hard to read. At least put in parentheses so we can have visual fix
    on what sizeof applies to.
     
    Malcolm McLean, Aug 24, 2013
    #13
  14. Sharwan Joram

    Sven Köhler Guest

    Hi,

    here are some comments on your code.

    Am 23.08.2013 20:15, schrieb Sharwan Joram:
    I can't tell how much memory you allocate. What's the value of
    parametercount?
    Clearly this line is wrong. You cast to (char*), yet you use
    sizeof(char*). What you really meant is

    parameters[parametercount] = (char *)malloc(30 * sizeof (char));
    I hope you make sure, each token is no longer than 29 characters.
    Also instead of using memset, memcpy, and strlen, you could use strncpy:

    strncpy(parameters[parametercount], token, 29);
    parameters[parametercount][29]=0;
    You just allocated some memory with malloc. You will never be able to
    free that memory again, since you overwrite the pointer here. Also, you
    just memcpy'ed the string - so why copy the pointer? Delete this line.

    Also, a few lines below you free all pointers in the array. Actually,
    you won't free what you allocated here, but you will free the memory
    pointed to by the token variable! And from the looks of it, the token
    variable contains to the middle of a string. Such pointers cannot be free'd.
    I can't tell what the value of idx is. Also, it seems to me like you
    want to count backwards but instead you have ++parametercount in the for
    loop.

    Please post a snippet that is somewhat more complete.


    Regards,
    Sven
     
    Sven Köhler, Aug 24, 2013
    #14
  15. I've modified the code with the guidelines given by James and other mentorsin the list, mentioned below. But it's still failing.

    -----------Code Snippet -------------




    /*-----------------------------------------*/
    if(token != NULL){
    char *temp_token = (char *) malloc (strlen(saved_token) + 1);
    memcpy(temp_token, saved_token, strlen(saved_token));
    idx = parametercount = detect_delim_count(temp_token, delimiters);
    if (temp_token)
    free(temp_token);
    parameters = malloc(parametercount * sizeof(char *));
    if ( NULL == parameters){
    fprintf(stdout, "CLI: Couldn't allocate sufficient memory . Exiting \n");
    exit(1);
    }
    for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    parameters[parametercount] = calloc(30 , strlen(token) + 1);
    if ( NULL == parameters[parametercount]){
    fprintf(stdout,"CLI: Couldn't allocate sufficient memory. Exiting \n");
    exit(1);
    }
    strncpy(parameters[parametercount], token, strlen(token));
    }
    shutdown = (currentcommand->command_handler)(client_fd, parameters, parametercount);
    }
    }else{
    /* some more code here */

    }
    if (parameters != NULL){
    for (parametercount = idx ; parametercount >= 0; --parametercount)
    free(parameters[parametercount]);
    free(parameters);
    }




    int detect_delim_count(char *i_str, char *delim)
    {
    char *itr_str = NULL;
    char *parser = NULL;
    int count = 0;

    itr_str = i_str;
    for (parser = strtok(itr_str, delim); parser && *parser ; parser = strtok(NULL, delim))
    count++;

    return (count-1);
    }

    ---------------DEBUG TRACE-----------
    Breakpoint 1, execute_commands (client_fd=10, command_name=0xb27fef54 "show parameters param1 param2 param3 param4", d_len=44) at opennopd/subsystems/clicommands.c:158
    158 if(token != NULL){
    Missing separate debuginfos, use: debuginfo-install glibc-2.15-59.fc17.i686libmnl-1.0.3-4.fc17.i686 libnetfilter_queue-1.0.2-1.fc17.i686 libnfnetlink-1.0.1-1.fc17.i686 nss-softokn-freebl-3.14.3-1.fc17.i686
    (gdb) n
    159 char *temp_token = (char *) malloc (strlen(saved_token) + 1);
    (gdb)
    160 memcpy(temp_token, saved_token, strlen(saved_token));
    (gdb)
    161 idx= parametercount = detect_delim_count(temp_token, delimiters);
    (gdb)
    162 if (temp_token)
    (gdb)
    163 free(temp_token);
    (gdb) p idx
    $1 = 3
    (gdb) n
    164 parameters = malloc(parametercount * sizeof(char *));
    (gdb)
    165 if ( NULL == parameters){
    (gdb)
    169 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb)
    170 parameters[parametercount] = calloc(30 , strlen(token) + 1);
    (gdb)
    171 if ( NULL == parameters[parametercount]){
    (gdb)
    175 strncpy(parameters[parametercount], token, strlen(token));
    (gdb)
    169 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb)
    170 parameters[parametercount] = calloc(30 , strlen(token) + 1);
    (gdb)
    171 if ( NULL == parameters[parametercount]){
    (gdb)
    169 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb)
    170 parameters[parametercount] = calloc(30 , strlen(token) + 1);
    (gdb)
    171 if ( NULL == parameters[parametercount]){
    (gdb)
    175 strncpy(parameters[parametercount], token, strlen(token));
    (gdb)
    169 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb)
    170 parameters[parametercount] = calloc(30 , strlen(token) + 1);
    (gdb)
    171 if ( NULL == parameters[parametercount]){
    (gdb)
    175 strncpy(parameters[parametercount], token, strlen(token));
    (gdb)
    169 for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
    (gdb)
    177 shutdown = (currentcommand->command_handler)(client_fd, parameters, parametercount);
    (gdb)
    188 executedcommand = currentcommand;
    (gdb) p parameters[0]
    $2 = 0xb6e0a040 "param1"
    (gdb) p parameters[1]
    $3 = 0xb6e0a118 "param2"
    (gdb) p parameters[2]
    $4 = 0xb6e0a1f0 "param3"
    (gdb) p parameters[3]
    $5 = 0xb6e0a2c8 "param4"
    (gdb) p parameters[4]
    $6 = 0x61726170 <Address 0x61726170 out of bounds>
    (gdb) p *parameters
    $7 = 0xb6e0a040 "param1"
    (gdb) n
    193 currentcommand = currentcommand->next;
    (gdb)
    127 while((executedcommand == NULL)&& (currentcommand != NULL)){
    (gdb)
    198 if (parameters != NULL){
    (gdb)
    199 for (parametercount = idx; parametercount >= 0; --parametercount)
    (gdb)
    200 free(parameters[parametercount]);
    (gdb) p idx
    $8 = 3
    (gdb) n
    *** glibc detected *** /home/sources/opennop-daemon/opennopd/opennopd: corrupted double-linked list: 0xb6e0a028 ***
    ======= Backtrace: =========
    /lib/libc.so.6[0x4441a1d1]
    /lib/libc.so.6[0x4441a7bd]
    /home/sources/opennop-daemon/opennopd/opennopd[0x80529e2]
    /home/sources/opennop-daemon/opennopd/opennopd[0x80520f5]
    /lib/libpthread.so.0[0x4455fadf]
    /lib/libc.so.6(clone+0x5e)[0x4449944e]
    ======= Memory map: ========
    08048000-08056000 r-xp 00000000 fd:01 397491 /home/sources/opennop-daemon/opennopd/opennopd
    08056000-08057000 rw-p 0000d000 fd:01 397491 /home/sources/opennop-daemon/opennopd/opennopd
    08057000-082ce000 rw-p 00000000 00:00 0 [heap]
    44382000-443a1000 r-xp 00000000 fd:01 148916 /usr/lib/ld-2.15.so
    443a1000-443a2000 r--p 0001e000 fd:01 148916 /usr/lib/ld-2.15.so
    443a2000-443a3000 rw-p 0001f000 fd:01 148916 /usr/lib/ld-2.15.so
    443a5000-44550000 r-xp 00000000 fd:01 148917 /usr/lib/libc-2.15.so
    44550000-44551000 ---p 001ab000 fd:01 148917 /usr/lib/libc-2.15.so
    44551000-44553000 r--p 001ab000 fd:01 148917 /usr/lib/libc-2.15.so
    44553000-44554000 rw-p 001ad000 fd:01 148917 /usr/lib/libc-2.15.so
    44554000-44557000 rw-p 00000000 00:00 0
    44559000-4456f000 r-xp 00000000 fd:01 148918 /usr/lib/libpthread-2.15.so
    4456f000-44570000 r--p 00015000 fd:01 148918 /usr/lib/libpthread-2.15.so
    44570000-44571000 rw-p 00016000 fd:01 148918 /usr/lib/libpthread-2.15.so
    44571000-44573000 rw-p 00000000 00:00 0
    44575000-44578000 r-xp 00000000 fd:01 148924 /usr/lib/libdl-2.15.so
    44578000-44579000 r--p 00002000 fd:01 148924 /usr/lib/libdl-2.15.so
    44579000-4457a000 rw-p 00003000 fd:01 148924 /usr/lib/libdl-2.15.so
    448f5000-44911000 r-xp 00000000 fd:01 148934 /usr/lib/libgcc_s-4.7.2-20120921.so.1
    44911000-44912000 rw-p 0001b000 fd:01 148934 /usr/lib/libgcc_s-4.7.2-20120921.so.1
    45642000-45692000 r-xp 00000000 fd:01 148937 /usr/lib/libfreebl3.so
    45692000-45693000 rw-p 00050000 fd:01 148937 /usr/lib/libfreebl3.so
    45693000-45697000 rw-p 00000000 00:00 0
    456a8000-456b0000 r-xp 00000000 fd:01 148938 /usr/lib/libcrypt-2.15.so
    456b0000-456b1000 r--p 00007000 fd:01 148938 /usr/lib/libcrypt-2.15.so
    456b1000-456b2000 rw-p 00008000 fd:01 148938 /usr/lib/libcrypt-2.15.so
    456b2000-456d9000 rw-p 00000000 00:00 0
    b1fff000-b2000000 ---p 00000000 00:00 0
    b2000000-b2800000 rw-p 00000000 00:00 0 [stack:4641]
    b2800000-b2821000 rw-p 00000000 00:00 0
    b2821000-b2900000 ---p 00000000 00:00 0
    b2a00000-b2b00000 rw-p 00000000 00:00 0
    b2bf9000-b2bfa000 ---p 00000000 00:00 0
    b2bfa000-b33fa000 rw-p 00000000 00:00 0 [stack:4637]
    b33fa000-b33fb000 ---p 00000000 00:00 0
    b33fb000-b3bfb000 rw-p 00000000 00:00 0 [stack:4636]
    b3bfb000-b3bfc000 ---p 00000000 00:00 0
    b3bfc000-b43fc000 rw-p 00000000 00:00 0 [stack:4635]
    b43fc000-b43fd000 ---p 00000000 00:00 0
    b43fd000-b4bfd000 rw-p 00000000 00:00 0 [stack:4634]
    b4bfd000-b4bfe000 ---p 00000000 00:00 0
    b4bfe000-b53fe000 rw-p 00000000 00:00 0 [stack:4633]
    b53fe000-b53ff000 ---p 00000000 00:00 0
    b53ff000-b5bff000 rw-p 00000000 00:00 0 [stack:4632]
    b5bff000-b5c00000 ---p 00000000 00:00 0
    b5c00000-b6400000 rw-p 00000000 00:00 0 [stack:4631]
    b6400000-b6500000 rw-p 00000000 00:00 0
    b65ff000-b6600000 ---p 00000000 00:00 0
    b6600000-b6e00000 rw-p 00000000 00:00 0 [stack:4630]
    b6e00000-b6e2a000 rw-p 00000000 00:00 0
    b6e2a000-b6f00000 ---p 00000000 00:00 0
    b6fd2000-b6fd3000 ---p 00000000 00:00 0
    b6fd3000-b77d3000 rw-p 00000000 00:00 0 [stack:4629]
    b77d3000-b77d4000 ---p 00000000 00:00 0
    b77d4000-b7fd6000 rw-p 00000000 00:00 0 [stack:4628]
    b7fd6000-b7fda000 r-xp 00000000 fd:01 165010 /usr/lib/libmnl.so.0.1.0
    b7fda000-b7fdb000 r--p 00003000 fd:01 165010 /usr/lib/libmnl.so.0.1.0
    b7fdb000-b7fdc000 rw-p 00004000 fd:01 165010 /usr/lib/libmnl.so.0.1.0
    b7fdc000-b7fe2000 r-xp 00000000 fd:01 165001 /usr/lib/libnfnetlink.so.0..2.0
    b7fe2000-b7fe3000 r--p 00005000 fd:01 165001 /usr/lib/libnfnetlink.so.0..2.0
    b7fe3000-b7fe4000 rw-p 00006000 fd:01 165001 /usr/lib/libnfnetlink.so.0..2.0
    b7fe4000-b7fe5000 rw-p 00000000 00:00 0
    b7fe5000-b7feb000 r-xp 00000000 fd:01 148876 /usr/lib/libnetfilter_queue.so.1.3.0
    b7feb000-b7fec000 r--p 00005000 fd:01 148876 /usr/lib/libnetfilter_queue.so.1.3.0
    b7fec000-b7fed000 rw-p 00006000 fd:01 148876 /usr/lib/libnetfilter_queue.so.1.3.0
    b7ffc000-b7fff000 rw-p 00000000 00:00 0
    b7fff000-b8000000 r-xp 00000000 00:00 0 [vdso]
    bffdf000-c0000000 rw-p 00000000 00:00 0 [stack]

    Program received signal SIGABRT, Aborted.
    0xb7fff424 in __kernel_vsyscall ()
    Missing separate debuginfos, use: debuginfo-install libgcc-4.7.2-2.fc17.i686
    (gdb)

    Input is : param1 param2 param3 param4

    Thanks
    Sharwan
     
    Sharwan Joram, Aug 24, 2013
    #15
  16. What are the options? Are you assuming a reader who thinks sizeof might
    be a variable?
     
    Ben Bacarisse, Aug 24, 2013
    #16
  17. Sharwan Joram

    blmblm Guest

    [ snip ]
    I'll second this recommendation to the OP!! It seems that much of
    the difficulty in solving the problem has to do with variables whose
    values readers can't be sure about. And as you say, sometimes just
    trying to pull out a short self-contained program that demonstrates
    the problem helps in identifying it.

    OP: Is there some reason you can't do that??

    [ snip ]
     
    blmblm, Aug 24, 2013
    #17
  18. At this point, temp_token does not point to a string. You forgot to
    copy the terminal \0. Once again, I suggest you use the appropriate
    function (strcpy in this case) rather than trying to use one that you
    have to force to be suitable.
    This test is too late. If temp_token is NULL, the call to memcpy and
    the processing in detect_delim_count both invoke undefined behavior.
    1 is not a portable return code from main. I suggest you use
    EXIT_FAILURE.
    The test on *token is still completely superfluous.

    Why do you assign a value to parameter count above only to reset the
    value here?
    I'm confused. I token points to a string of len 5, you allocate and
    zero 180 bytes and then copy the 5 char string into the allocated
    space. What is the extra 174 bytes for?

    strcpy would be more efficient than strncpy for copying the string
    unless you change the third argument to something useful.
    This test is too late. If parameters is NULL at this point, you have
    already invoked undefined behavior multiple times.
    Why -1? Is it your intent to never process the last tokenized string?
     
    Barry Schwarz, Aug 24, 2013
    #18
  19. It's perfectly easy to read once you've learned it, and you only have to
    learn it once. Don't try to prevent other people from learning the
    language.
     
    Keith Thompson, Aug 24, 2013
    #19
  20. [...]

    See also http://sscce.org/ ("Short, Self Contained, Correct
    (Compilable), Example").
     
    Keith Thompson, Aug 24, 2013
    #20
    1. Advertisements

Ask a Question

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

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.