Memory corruption on freeing a pointer to pointer

[Sharwan Joram]

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

 

[James Kuyper]

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.

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.

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);

It's simpler, and on some systems, faster, to call calloc() rather than
malloc() followed by memset().

memcpy(parameters[parametercount], token, strlen(token));

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.

parameters[parametercount] = token;

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.

}

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

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.

 

[Sharwan Joram]

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.

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.

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);

It's simpler, and on some systems, faster, to call calloc() rather than

malloc() followed by memset().

memcpy(parameters[parametercount], token, strlen(token));

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.

parameters[parametercount] = token;

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.

}

/* idx contains the number of tokens */

if (parameters != NULL){

for (parametercount = idx; parametercount >= 0; ++parametercount)

free(parameters[parametercount]);

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.

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

 

[Martin Shobe]

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.

What do you think it breaks?

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

I'll assume you left out the code that initialized saved_token.

parameters[parametercount] = (char *)malloc(30 * sizeof (char *));
memset(parameters[parametercount], '\0', 30);
memcpy(parameters[parametercount], token, strlen(token));

If you're going to use strlen(token), why not go ahead and use strcpy?

parameters[parametercount] = token;

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.

}

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

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

 

[James Kuyper]

On 08/23/2013 01:15 PM, Sharwan Joram wrote: ....

parameters[parametercount] = token;

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.

}

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

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.

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

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.

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.

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.

 

[Ike Naar]

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

parameters = (char **)malloc(parametercount * sizeof(char *));
// Don't use *parameters it breaks.

What is the value of parametercount?

 

[Ben Bacarisse]

What is the value of parametercount?

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.

 

[Barry Schwarz]

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.

The value of parametercount is indeterminant. The next block of code
implies it is not known at this time.

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

What does saved_token point to? Under what condition can token be
non-NULL and *token be '\0'?

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

This allocates too much memory.

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

What purpose does this serve since you immediately replace the zeroed
out values in the next statement?

memcpy(parameters[parametercount], token, strlen(token));

Why not use strcpy and save at least one function call?

parameters[parametercount] = token;

You have now caused a memory leak. parameters[parametercount] used to
point to the allocated memory. Now it point to somewhere inside
saved_token.

}

/* idx contains the number of tokens */
if (parameters != NULL){

If parameters is NULL, all the previous code invokes undefined
behavior. Essentially, this is if statement must evaluate to true.

for (parametercount = idx; parametercount >= 0; ++parametercount)

If idx is positive, this loop will never terminate.

free(parameters[parametercount]);

Since parameters[parametercount] no longer contains a value returned
by malloc, this call to free invokes undefined behavior and frequently
causes programs to crash.

free(parameters);
}

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

This trace does not match the code you have shown us.

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)

An assignment statement should have been traced here.

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

<snip>

Show us your real code.

 

[James Kuyper]

....

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.

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

This allocates too much memory.

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.

If idx is positive, this loop will never terminate.

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)

 

[gdotone]

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);

....

 

[Malcolm McLean]

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?

 

[Ben Bacarisse]

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 * ) );

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.

/*
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)

You have the wrong type for token. It, too, should be a char *.

parameter is initialized to 0

No it isn't. Did you mean parameterCount? You need to take a lot of
care in programming. Little details make all the difference.

token is initialized to whatever *saved_token
first token is.
( but *saved_token is not initialized at
this point )

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.

++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," ") );

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.

{

/*
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 *));

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.

/*
void *memset( void *s, int c, size_t n );

The header files are there for a purpose! It's rarely a good idea to
manually declare C library functions like this.

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?

No. parameters[parameterCount] is just a pointer to a character.

so, in the 30 position of the array of characters place a '\0' character

*/

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

I suggest you try to write a cut-down version of this program and post
it whole.

 

[Malcolm McLean]

(e-mail address removed) writes:



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.

But it's hard to read. At least put in parentheses so we can have visual fix
on what sizeof applies to.

 

[Sven Köhler]

Hi,

here are some comments on your code.

Am 23.08.2013 20:15, schrieb Sharwan Joram:

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.

I can't tell how much memory you allocate. What's the value of
parametercount?

for( parametercount = 0 , token = strtok(saved_token, " "); token && *token ; ++parametercount , token = strtok(NULL, " ")){
parameters[parametercount] = (char *)malloc(30 * sizeof (char *));

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));

memset(parameters[parametercount], '\0', 30);
memcpy(parameters[parametercount], token, strlen(token));

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;

parameters[parametercount] = token;

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.

}

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

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

 

[Sharwan Joram]

Hi,



here are some comments on your code.



Am 23.08.2013 20:15, schrieb Sharwan Joram:

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.

I can't tell how much memory you allocate. What's the value of

parametercount?

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

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

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));

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

memcpy(parameters[parametercount], token, strlen(token));

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;

parameters[parametercount] = token;

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.

}

/* idx contains the number of tokens */

if (parameters != NULL){

for (parametercount = idx; parametercount >= 0; ++parametercount)

free(parameters[parametercount]);

free(parameters);

}

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

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

 

[Ben Bacarisse]

But it's hard to read. At least put in parentheses so we can have visual fix
on what sizeof applies to.

What are the options? Are you assuming a reader who thinks sizeof might
be a variable?

 

[blmblm]

[ snip ]

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

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.

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 ]

 

[Barry Schwarz]

I've modified the code with the guidelines given by James and other mentors in 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));

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.

idx = parametercount = detect_delim_count(temp_token, delimiters);
if (temp_token)

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.

free(temp_token);
parameters = malloc(parametercount * sizeof(char *));
if ( NULL == parameters){
fprintf(stdout, "CLI: Couldn't allocate sufficient memory . Exiting \n");
exit(1);

1 is not a portable return code from main. I suggest you use
EXIT_FAILURE.

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

The test on *token is still completely superfluous.

Why do you assign a value to parameter count above only to reset the
value here?

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));

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.

}
shutdown = (currentcommand->command_handler)(client_fd, parameters, parametercount);
}
}else{
/* some more code here */

}
if (parameters != NULL){

This test is too late. If parameters is NULL at this point, you have
already invoked undefined behavior multiple times.

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);

Why -1? Is it your intent to never process the last tokenized string?

 

[Keith Thompson]

But it's hard to read. At least put in parentheses so we can have visual fix
on what sizeof applies to.

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]

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.

[...]

See also http://sscce.org/ ("Short, Self Contained, Correct
(Compilable), Example").