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]

On 08/23/2013 01:15 PM, Sharwan Joram wrote:
> 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 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]

On 8/23/2013 12:15 PM, Sharwan Joram wrote:
> 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 02:17 PM, Sharwan Joram wrote:
> On Friday, August 23, 2013 11:35:23 PM UTC+5:30, James Kuyper wrote:
>> 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]

On 2013-08-23, Sharwan Joram <sharwa...@gmail.com> wrote:
> 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]

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.

[Barry Schwarz]

On Fri, 23 Aug 2013 10:15:42 -0700 (PDT), Sharwan Joram
<sharwa...@gmail.com> wrote:

>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.

--
Remove del for email

[James Kuyper]

On 08/23/2013 05:16 PM, Barry Schwarz wrote:
> On Fri, 23 Aug 2013 10:15:42 -0700 (PDT), Sharwan Joram
> <sharwa...@gmail.com> wrote:
>
>> 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 :

...

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.

>> for (parametercount = idx; parametercount >= 0; ++parametercount)
>
> 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)

[gdo...@gmail.com]

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]

On Friday, August 23, 2013 6:15:42 PM UTC+1, Sharwan Joram wrote:
>
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]

gdo...@gmail.com writes:

> 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.

--
Ben.

[Malcolm McLean]

On Saturday, August 24, 2013 1:45:45 PM UTC+1, Ben Bacarisse wrote:

> gdo...@gmail.com writes:
>

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

> 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]

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

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.i686 libmnl-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

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]

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

--
Ben.

[blmblm.m...@gmail.com]

In article <5217AF49...@verizon.net>,

James Kuyper <james...@verizon.net> wrote:
> On 08/23/2013 02:17 PM, Sharwan Joram wrote:
> > On Friday, August 23, 2013 11:35:23 PM UTC+5:30, James Kuyper wrote:
> >> On 08/23/2013 01:15 PM, Sharwan Joram wrote:

[ 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 ]

--
B. L. Massingill
ObDisclaimer: I don't speak for my employers; they return the favor.

[Barry Schwarz]

On Sat, 24 Aug 2013 12:05:02 -0700 (PDT), Sharwan Joram
<sharwa...@gmail.com> wrote:

>On Saturday, August 24, 2013 7:32:52 PM UTC+5:30, Sven Köhler wrote:

<snip a lot of irrelevant previous messages>

>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]

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 (The_Other_Keith) ks...@mib.org <http://www.ghoti.net/~kst>
Working, but not speaking, for JetHead Development, Inc.
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

[Keith Thompson]

James Kuyper <james...@verizon.net> writes:
[...]

> 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").

[Keith Thompson]

Barry Schwarz <schw...@dqel.com> writes:
> On Sat, 24 Aug 2013 12:05:02 -0700 (PDT), Sharwan Joram
> <sharwa...@gmail.com> wrote:

[...]

>> strncpy(parameters[parametercount], token, strlen(token));
>

[...]

>
> strcpy would be more efficient than strncpy for copying the string
> unless you change the third argument to something useful.

And strncpy is *not* just a "safer" strcpy; it can leave the target
array without a terminating '\0' character (i.e., not a string).

[Malcolm McLean]

On Saturday, August 24, 2013 8:16:00 PM UTC+1, Ben Bacarisse wrote:

> Malcolm McLean <malcolm...@btinternet.com> writes:
>
>
> What are the options? Are you assuming a reader who thinks sizeof might
> be a variable?
>

A bad option is

buff = malloc( N * sizeof *buff);

it's got two C quirks - an operator which looks like an identifier, and a
sort of pointer dereference which isn't really a dereference. It also uses the
asterisk in two contexts, which aren't visually distinguishable. It's asking
for trouble, difficulty in understanding, adding cost to the maintenance of
programs.

buff = malloc( N * sizeof(*buff));

is a bit less bad. At least we can see that sizeof applies to *buff, and that
* is unary. A novice or an experience programmer unused to C might assume
that sizeof() is a fucntion, but that's unlikely to do any real harm.

The best option is

buff = malloc(N * sizeof(int));

it's very easy to see what the intention is.

[Ian Collins]

Malcolm McLean wrote:
> On Saturday, August 24, 2013 8:16:00 PM UTC+1, Ben Bacarisse wrote:
>> Malcolm McLean <malcolm...@btinternet.com> writes:
>>
>>
>> What are the options? Are you assuming a reader who thinks sizeof might
>> be a variable?
>>
>
> A bad option is
>
> buff = malloc( N * sizeof *buff);
>
> it's got two C quirks - an operator which looks like an identifier, and a
> sort of pointer dereference which isn't really a dereference. It also uses the
> asterisk in two contexts, which aren't visually distinguishable. It's asking
> for trouble, difficulty in understanding, adding cost to the maintenance of
> programs.

If a so called programmer doesn't understand this idiomatic construct,
they shouldn't be working on C code.

> buff = malloc( N * sizeof(*buff));

Most programmers would be happy with and probably use this form.

> is a bit less bad. At least we can see that sizeof applies to *buff, and that
> * is unary. A novice or an experience programmer unused to C might assume
> that sizeof() is a fucntion, but that's unlikely to do any real harm.
>
> The best option is
>
> buff = malloc(N * sizeof(int));
>
> it's very easy to see what the intention is.

Until the type of buff changes, which does happen.

--
Ian Collins

[Malcolm McLean]

On Sunday, August 25, 2013 1:27:25 AM UTC+1, Ian Collins wrote:

> Malcolm McLean wrote:
>
> If a so called programmer doesn't understand this idiomatic construct,
> they shouldn't be working on C code.
>

A programmer can be a very experienced programmer, but not so familiar with C.
That's happening more and more with the explosion of high-level languages.
But even an experienced C programmer thinks in English (assuming him to be
an anglophone). "size of int" is prounceable in English, and has the correct
meaning. size of star peeteear doesn't have a meaning.
In programming the trivial is important. Little difficulties have a way of
accumulating, and combining to make major headaches.

>
> > buff = malloc(N * sizeof(int));
> > it's very easy to see what the intention is.
>
> Until the type of buff changes, which does happen.
>

Automatic variables shouldn't have a large scope, so you should be able to
check the entire function when making a modification. The real danger is
when buff is a member of a structure, which has many functions which operate
on it. But you can easily have other problems.

[Ben Bacarisse]

Sharwan Joram <sharwa...@gmail.com> writes:
<snip>

> I've modified the code with the guidelines given by James and other
> mentors in the list, mentioned below. But it's still failing.

Some things not yet mentioned...

> -----------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, " ")){

This loop can run off the end of the allocated array. You need to stop
before parameterCount == idx.

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

Here you free elements that you never set. Even if you used every
available element, the very first free call accesses memory outside the
array.

> free(parameters);
> }
<snip>

--
Ben.

[Ben Bacarisse]

Malcolm McLean <malcolm...@btinternet.com> writes:

> On Saturday, August 24, 2013 8:16:00 PM UTC+1, Ben Bacarisse wrote:
>> Malcolm McLean <malcolm...@btinternet.com> writes:
>>
>>
>> What are the options? Are you assuming a reader who thinks sizeof might
>> be a variable?
>>
>
> A bad option is
>
> buff = malloc( N * sizeof *buff);

No. My remark followed this (no snipped):

| At least put in parentheses so we can have visual fix on what sizeof
| applies to.

I meant what are the options for what sizeof may apply to? I can see
only three, and the wrong ones are so daft I would bother to address
them.

<snip>
--
Ben.

[Keith Thompson]

Malcolm McLean <malcolm...@btinternet.com> writes:
> On Saturday, August 24, 2013 8:16:00 PM UTC+1, Ben Bacarisse wrote:
>> Malcolm McLean <malcolm...@btinternet.com> writes:
>> What are the options? Are you assuming a reader who thinks sizeof might
>> be a variable?
>>
>
> A bad option is
>
> buff = malloc( N * sizeof *buff);
>
> it's got two C quirks - an operator which looks like an identifier, and a
> sort of pointer dereference which isn't really a dereference. It also uses the
> asterisk in two contexts, which aren't visually distinguishable. It's asking
> for trouble, difficulty in understanding, adding cost to the maintenance of
> programs.

You understand it perfectly well, don't you?

sizeof is unusual in that it's the only operator named by a keyword
rather than by a punctuation symbol. (C11 adds _Alignof, but that
doesn't take a subexpression operand.) There are other languages that
have multiple operators whose names are keywords. Even C has "and",
"or", "xor" and so on if you have #include <iso646.h>. It's really not
a difficult concept.

> buff = malloc( N * sizeof(*buff));
>
> is a bit less bad. At least we can see that sizeof applies to *buff, and that
> * is unary. A novice or an experience programmer unused to C might assume
> that sizeof() is a fucntion, but that's unlikely to do any real harm.

A programmer who thinks sizeof is a function needs to learn that sizeof
isn't a function. Once. You might try telling people that yourself,
rather than suggesting elaborate workarounds to avoid the burden of
learning it.

I have no great objection to extraneous parentheses on sizeof; they're
mostly harmless. But I dislike using them myself, because I prefer to
keep in mind that sizeof is an operator, not a function. (Same thing
for "return" and "case".)

> The best option is
>
> buff = malloc(N * sizeof(int));
>
> it's very easy to see what the intention is.

No, because I declared "int32_t *buff", and if I write it that way I'll
never notice the error until I try it on a 64-bit system.

(And someone with the level of expertise you assume will probably think
that sizeof is a function, and wonder what it means to evaluate "int" as
an argument.)

[Ian Collins]

Malcolm McLean wrote:
> On Sunday, August 25, 2013 1:27:25 AM UTC+1, Ian Collins wrote:
>> Malcolm McLean wrote:
>>
>> If a so called programmer doesn't understand this idiomatic construct,
>> they shouldn't be working on C code.
>>
> A programmer can be a very experienced programmer, but not so familiar with C.
> That's happening more and more with the explosion of high-level languages.

While true, that doesn't detract from my comment. programmers coming
from a scripting language probably aren't familiar with low level memory
management at all, so they have quite a bit to learn. The include C's
idioms. Suggesting experienced programmers avoid C idioms that may be
unfamiliar to others is a step too far.

> But even an experienced C programmer thinks in English (assuming him to be
> an anglophone). "size of int" is prounceable in English, and has the correct
> meaning. size of star peeteear doesn't have a meaning.
> In programming the trivial is important. Little difficulties have a way of
> accumulating, and combining to make major headaches.

Such as the types of variables changing?

>>> buff = malloc(N * sizeof(int));
>>> it's very easy to see what the intention is.
>>
>> Until the type of buff changes, which does happen.
>>
> Automatic variables shouldn't have a large scope, so you should be able to
> check the entire function when making a modification. The real danger is
> when buff is a member of a structure, which has many functions which operate
> on it. But you can easily have other problems.

I'd wager more dynamic allocations *are* used to initialise members of
structures (which are more likely to changer their type) than are used
for automatic variables. The only real use for dynamic allocations to
automatic variables is for objects that are too large for the (mythical)
stack.

--
Ian Collins

[Sharwan Joram]

On Sunday, August 25, 2013 6:40:45 AM UTC+5:30, Ben Bacarisse wrote:
> Sharwan Joram <sharwa...@gmail.com> writes:
>
> <snip>
>
> > I've modified the code with the guidelines given by James and other
>
> > mentors in the list, mentioned below. But it's still failing.
>
>
>
> Some things not yet mentioned...
>
>
>
> > -----------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, " ")){
>
>
>
> This loop can run off the end of the allocated array. You need to stop
>
> before parameterCount == idx.
>

idx contains the number of delimiters available in the string. For example if the string contains 2 delimiters then the sub-strings are 3. The idx decremented by -1 while returning because of the increment in it's value by 1 in last iteration of for loop. This loop works well as expected.

>
>
> > 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]);
>
>
>
> Here you free elements that you never set. Even if you used every
>
> available element, the very first free call accesses memory outside the
>
> array.

The elements are assigned above in the code. In for loop.
>
>
>
> > free(parameters);
>
> > }
>
> <snip>
>
>
>
> --
>
> Ben.

Let me know if you find anything else suspicious which can resolve this problem.

[Sven Köhler]

Dear Sharwan,

The code below doesn't crash and is tested with valgrind.
Learn from it.


#include <string.h>
#include <stdlib.h>
#include <stdio.h>

int detect_delim_count(char *i_str, const char *delim) {
char *token;
int count = 0;

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

return count;
}

int main() {

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

const char* saved_token = "d e f";
const char* delimiters = " ";

char *temp_token = (char *) malloc (strlen(saved_token) + 1);

if ( NULL == temp_token){

fprintf(stdout, "CLI: Couldn't allocate sufficient memory . Exiting \n");
exit(1);
}

strcpy(temp_token, saved_token);
parametercount = detect_delim_count(temp_token, delimiters);
strcpy(temp_token, saved_token);

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

for( idx = 0 , token = strtok(temp_token, " "); token && *token ; ++idx

, token = strtok(NULL, " ")){

parameters[idx] = malloc(strlen(token) + 1);
if ( NULL == parameters[idx]){

fprintf(stdout,"CLI: Couldn't allocate sufficient memory. Exiting \n");
exit(1);
}

fprintf(stdout, "found token %s\n", token);
strcpy(parameters[idx], token);
}
free(temp_token);
//shutdown = (currentcommand->command_handler)(client_fd, parameters,
parametercount);

while (parametercount > 0)
free(parameters[--parametercount]);
free(parameters);
}

[James Kuyper]

On 08/24/2013 03:05 PM, Sharwan Joram wrote:
...

> I've modified the code with the guidelines given by James and other mentors in the list, mentioned below. But it's still failing.

Not quite. As I and other have said, for best results you need to give
us a complete, compilable program. All you've given us is a:
> -----------Code Snippet -------------

While the code you have given us contains defects severe enough to be
the cause of the symptoms you've observed, you had no way of being sure
that would be the case. It could very well have been that the actual
problem was in some other part of your program.

> if(token != NULL){
> char *temp_token = (char *) malloc (strlen(saved_token) + 1);
> memcpy(temp_token, saved_token, strlen(saved_token));

Once again, you're using the value returned by malloc() before bothering
to check whether malloc() has succeeded. If malloc() failed, temp_token
would be null, and the call to memcpy() would render the behavior of
your program undefined.

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

Now you've finally bothered checking whether the allocation succeeded.
That's ironic, because you wait until after it is used twice in ways
that would be dangerous if temp_token is null. The only code you protect
is the call to free(), and free()ing a null pointer is actually
perfectly safe.

detect_delim_count() returns the number of delimiters. The number of
parameters is inherently one more than the number of delimiters. I see
from a later message that you're well aware of that fact. Therefore, I
find it puzzling that you would store the value returned by
detect_delim_count(), in a variable named parametercount. You should
either have named the variable delimitercount, or you should have added
1 to the value returned by detect_delim_count() before storing it in
parametercount.

> free(temp_token);
> parameters = malloc(parametercount * sizeof(char *));

Here you allocate enough room in the parameters array to store one
pointer for each delimiter. However, what you need is enough room to
store one pointer for each parameter, which is one more than the value
currently stored in parametercount.

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

You're now allocating enough space for 30 copies of the token. I think
you should change 30 to 1, unless there's some reason, not obvious in
the code you've shown, for setting aside that much space.

The last successful call to strtok() will result in the above code
attempting to store the value returned by malloc() into parameters[idx],
but you only allocated enough memory to store idx pointers:
parameters[0] through parameters[idx-1]. You haven't allocated enough
space to store anything in parameters[idx]. Attempting to do so renders
the behavior of your program undefined. Under certain circumstances,
your program might fail immediately because of this defect. However,
judging from the symptoms you've reported, what you did was corrupt some
memory being used by some other part of the program, almost certainly
memory being used by the malloc() family to manage the heap. That's why
the subsequent call to free() failed.

> if ( NULL == parameters[parametercount]){
> fprintf(stdout,"CLI: Couldn't allocate sufficient memory. Exiting \n");
> exit(1);
> }
> strncpy(parameters[parametercount], token, strlen(token));

strlen() has to examine every character in the string, to look for the
terminating null character. strncpy() needs to retrieve the value of
every character in the string. If you'd simply called strcpy() instead,
then it would only have gone through the string once.

In some contexts, strncpy(dest, src, count) can be safer than
strcpy(dest,src). That's true only when "count" is less than strlen(src)
and also less than the length of the array pointed at by dest.
strncpy(dest, src, strlen(src)) stops at exactly the same place that
strcpy() would stop - you don't get any protection at all. If
strlen(src) is bigger than the size of the array pointed at by dest,
strncpy(dest,src,strlen(src)) will still overwrite then end of that
array, just like strcpy().

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

You set parametercount initially to the same value as idx, and used
parametercount to determine how many parameters to allocate space for.
Unless code that you haven't shown has changed the value of idx since
that time, space was therefore only allocated for idx different
pointers, starting with parameters[0] and ending with parameters[idx-1].

However, the above loop starts by trying to retrieve the value of
parameters[idx] and passing it to free(). This would still be a separate
problem even if you corrected parametercount earlier to contain the
count of the parameters, rather than the count of delimiters. In that
case, parameters[idx] would refer to memory that had never been
allocated for use by your program, and which had never been written. to.
As a result, it might contain a trap representation, in which case even
retrieving the value of parameters[idx] would render the behavior of
your program undefined. Even it if were not a trap representation, it
almost certainly would not contain a value returned by a call to
malloc(). Therefore, passing that value to free() would also render the
behavior of your code undefined.

You should correct the for() loop to either of the following forms:

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

or

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

The second form is slightly shorter and more idiomatic. I'd also
recommend exchanging the variable names idx and parametercount, since
you're using parameter count more like an index into the parameters
array, and less like a fixed count of parameters.
--
James Kuyper

[Malcolm McLean]

On Sunday, August 25, 2013 4:03:07 AM UTC+1, Ian Collins wrote:
> Malcolm McLean wrote:
>
> > In programming the trivial is important. Little difficulties have a way of
> > accumulating, and combining to make major headaches.
>
> Such as the types of variables changing?
>

The fact that the sizeof *ptr method is robust to the type of ptr changing is
a real advantage. That has to be balanced against the disadvantage of writing
code which is difficult for a human to read.

>
> I'd wager more dynamic allocations *are* used to initialise members of
> structures (which are more likely to changer their type) than are used
> for automatic variables. The only real use for dynamic allocations to
> automatic variables is for objects that are too large for the (mythical)
> stack.
>

Stacks are getting very big now. I wouldn't have considered declaring a
buffer of 1000 ints on the stack when I started with C, now I routinely do
so.

[Ben Bacarisse]

Sharwan Joram <sharwa...@gmail.com> writes:
> On Sunday, August 25, 2013 6:40:45 AM UTC+5:30, Ben Bacarisse wrote:
>> Sharwan Joram <sharwa...@gmail.com> writes:
>> <snip>
>>
>> > I've modified the code with the guidelines given by James and other
>> > mentors in the list, mentioned below. But it's still failing.
>>
>> Some things not yet mentioned...

<snip>

>> > if(token != NULL){
>> > char *temp_token = (char *) malloc (strlen(saved_token) + 1);
>> > memcpy(temp_token, saved_token, strlen(saved_token));

You need to copy one more byte to ensure the string is null-terminated,
though, in fact, a simple strcpy(temp_token, saved_token); call is
better here.

>> > if (temp_token)
>> > free(temp_token);

This test is a bit late. I'd put it here:

char *temp_token = malloc(strlen(saved_token) + 1);
if (temp_token) {
strcpy(temp_token, saved_token);

idx = parametercount = detect_delim_count(temp_token, delimiters);

free(temp_token);
}
else { /* complain and exit */ }

>> > 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, " ")){

You count delimiters using a variable "delimiters", but here you use
" ". This may be fine for you test case but it's not right in general
and could do all kinds of damage.

>> This loop can run off the end of the allocated array. You need to stop
>> before parameterCount == idx.
>
> idx contains the number of delimiters available in the string. For
> example if the string contains 2 delimiters then the sub-strings are
> 3. The idx decremented by -1 while returning because of the increment
> in it's value by 1 in last iteration of for loop. This loop works well
> as expected.

The one in detect_delim_count works but this one doesn't.

I'd missed the counting, but it still does not work. When there are no
delimiters, idx is set to 0 and you call malloc(0). That's wrong, but
you then make matters worse by assigning to parameters[0]. In the
general case, you count the delimiters but then store a string for every
token (that's one more than the number of delimiters). You always
overrun this array.

I suspect you've got confused as to whether you choose to count
delimiters of parameters. Personally, I'd count parameters.

Given that you've already counted either delimiters (or parameters), I
would suggest you make it clear by making this loop a simple counting
one:

for (int i = 0; i < idx; i++) {
const char *tok = strtok(i == 0 ? saved_token : NULL, " ");
cont int size_t = strlen(tok);
parameters[i] = malloc(len + 1);
// Failure test removed to reduce my typing.
strcpy(parameters[i], tok);
}

I've also removed the odd call to calloc that allocates 30 times the
storage that's needed. Instead, I allocate only what's needed for each
string and so the copy with a plain strcpy.

<snip>

>> > if (parameters != NULL){
>> > for (parametercount = idx ; parametercount >= 0; --parametercount)
>> > free(parameters[parametercount]);
>>
>> Here you free elements that you never set. Even if you used every
>> available element, the very first free call accesses memory outside the
>> array.
>
> The elements are assigned above in the code. In for loop.

Not parameters[idx]. Well, maybe I should be more explicit. You
allocate room for idx pointers indexed from 0 to idx-1, so either the
first loop will overrun the array (by assigning to parameters[idx]), or,
when that's fixed, this loop frees a pointer that was never set.

>> > free(parameters);
>> > }
>>
>> <snip>
>>
>>
>>
>> --
>>
>> Ben.
>
> Let me know if you find anything else suspicious which can resolve
> this problem.

If you worked on making the code clearer, i.e. writing just what you
mean, I think you'd have been able to find most of these yourself.

A tip: can you run the code under a memory checker like valgrind? It's
found more bugs in my code than any other debug tool. I use it whenever
I can.

--
Ben.

[Keith Thompson]

Malcolm McLean <malcolm...@btinternet.com> writes:
> On Sunday, August 25, 2013 4:03:07 AM UTC+1, Ian Collins wrote:
>> Malcolm McLean wrote:
>> > In programming the trivial is important. Little difficulties have a way of
>> > accumulating, and combining to make major headaches.
>>
>> Such as the types of variables changing?
>>
> The fact that the sizeof *ptr method is robust to the type of ptr changing is
> a real advantage.

I'm glad you realize that.

> That has to be balanced against the disadvantage of writing
> code which is difficult for a human to read.

No, it doesn't. Learn the idiom once, understand it forever.

Do *you* find it difficult, or do you merely assume that everyone
else does? (I've asked you this, in different words, several times;
you have yet to answer.)

If your goal is to write code that can be understood by
non-programmers, or by programmers unfamiliar with the language
it's written in, I suggest C is not the language for you. Cobol was
meant to achieve that goal (no comment on whether it succeeded).

[...]

[Keith Thompson]

James Kuyper <james...@verizon.net> writes:
> On 08/24/2013 03:05 PM, Sharwan Joram wrote:

[...]

>> if ( NULL == parameters[parametercount]){

This is what's known as a "Yoda conndition"
<http://en.wikipedia.org/wiki/Yoda_Conditions>. I know that a lot of
programmers like them, and for somewhat valid reasons, but personally I
find them jarring and unnecessary. Personally, I'd write that as:

if (parameters[parametercount] == NULL) {

>> fprintf(stdout,"CLI: Couldn't allocate sufficient memory. Exiting \n");
>> exit(1);
>> }
>> strncpy(parameters[parametercount], token, strlen(token));
>
> strlen() has to examine every character in the string, to look for the
> terminating null character. strncpy() needs to retrieve the value of
> every character in the string. If you'd simply called strcpy() instead,
> then it would only have gone through the string once.
>
> In some contexts, strncpy(dest, src, count) can be safer than
> strcpy(dest,src). That's true only when "count" is less than strlen(src)
> and also less than the length of the array pointed at by dest.
> strncpy(dest, src, strlen(src)) stops at exactly the same place that
> strcpy() would stop - you don't get any protection at all. If
> strlen(src) is bigger than the size of the array pointed at by dest,
> strncpy(dest,src,strlen(src)) will still overwrite then end of that
> array, just like strcpy().

[...]

You missed the most dangerous thing about strncpy(): it doesn't
necessarily null-terminate the target array (and in this case it won't).
It's *not* just a safer strcpy().

http://the-flat-trantor-society.blogspot.com/2012/03/no-strncpy-is-not-safer-strcpy.html

[James Kuyper]

You're right - I deliberately didn't comment on that issue in his
original code, which always memset() the first 30 bytes. As a result, if
code which used those parameters was always careful to read either until
the 30th byte had been read, or until a null character was read,
whichever comes first, the failure to null-terminate the parameters
could have been correct (though I think it unlikely, particularly in the
context of the other errors in his code).

However, when he followed my suggestion to remove the memset(), but
failed to follow my suggestion to change strncpy() to strcpy(), that
removed any possibility that the rest of his code could deal with
parameters stored in this fashion. I should have reconsidered the issue.
--
James Kuyper

[gdo...@gmail.com]

On Friday, August 23, 2013 1:15:42 PM UTC-4, Sharwan Joram wrote:
> 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

> 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


/* small steps may help */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define MAXNUMBER 5

int main( void )
{

char **a; /* a pointer to a pointer of type char */


int numberOfPointers = 5;

/* a points to an allocated block of memory which are pointers */
/* in this case 5 such pointer. */

a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );


/* check to see if space was allocated */
/* and print a message if it was */

if ( a!= NULL )
printf( "the array, a, points to declare space\n\n" );


/* declare and iniitialized an array of pointers to characters */

char *b[] = { "one", "two", "three", "four", "five" };


/* pointer assignment */

a = b;


/*
using the pointer to pointer of type char, a, print
the strings of character array b.
*/

for ( int index = 0; index < 5; index++ )
{
printf( "%s\n\n", *(a+index) );
}


/*
find the size of a sting of characters
*/


char *string = "word" ;

size_t n = strlen( string );

printf("\n %zd \n\n", n); /* %zd because n is of type size_t */


/*
declare an array of characters, sentence[]
declare a token, which is a pointer to a string constant, " ".
declare a pointer to a character.

*/


char sentence[] = "this is a sentence with words";
char *token = " ";
char *g;

/*
grab the first word of the character array sentence.

char *strtok( char *s1, const char *s2 );
strtok needs an array of character passed as char *s1 on my machine. when a pointer to a string constant is use
a memory problem occurs.

get the length of the first word, after a call to strtok().
size_t strlen( const char *s );
print the length of that string, the first word in the arrary of characters, with a message.
*/

g = strtok(sentence, token);

size_t y = strlen(g);

printf("the length of the first word in the sentence is %d\n\n", (int) y );

/*
using the length of the string, of the first word, from the char sentence array,
char sentence[], allocate some space that will hold characters plus one more. assign a pointer,
c_space, char *c_space, to point to that created space.

print a message if space was allocated.
*/


char *c_space = (char *) malloc( y * sizeof(char) + 1 );

if (c_space)
{
printf("got space for word\n\n");
}



/* create and array using the length of the first word plus one copy */

int indexnewstring = (int) y + 1; /* (int) strlen( g ) + 1 */

char newstring[indexnewstring];


/* copy the first word into newstring and print string */

char *v = strcpy( newstring, g);
printf(" %s\n", v );


/*
create a second sentence array, char sentence2[], and through the sentence using a token to
separate each word.

*/
char sentence2[] = "this is a sentence with words";
g = strtok( sentence2, token );


while ( g != NULL )
{
printf("\n %s", g );
g = strtok( NULL, token );

}

printf("\n");

char sentence3[] = "C programming is fun execpt for pointers, actually it's the most fun";
for( g = strtok(sentence3, token ); g != NULL; g = strtok( NULL, token ) )
printf (" %s\n", g );




return 0;
}

[Keith Thompson]

gdo...@gmail.com writes:
[huge snip]

Your articles are difficult to read, partly because you're using the
broken Google Groups interface to Usenet.

GG encourages (or at least fails to discourage) very long lines, and it
handles quoted text poorly. Due, I think, to an assumption that
newlines denote paragraph breaks rather than line breaks, quoted text
tends to be double-spaced -- and quoted quoted text tends to be
quadruple-spaced, and so on.

It's better to keep your physical lines of text below 80 columns,
preferably about 72 to allow for insertion of "> > " in followups. And
if you can copy-and-paste an article into an editor, clean up the
double-spacing, and then copy-and-paste it back into your web browser
before posting, that would also be helpful.

And this is more of a style point, but in the article to which I'm now
replying you have a lot of C code (not quoted from a previous article)
with IMHO excessive blank lines. Some blank lines to show structure are
good, but too many just make it difficult to see more than a fraction of
the code on one screen.

Consider using an actual Usenet server rather than using Google Groups.
I use news.eternal-september.org, which is both good and free. You'd
need a Usenet client program. Mozilla Thunderbird works reasonably
well. I use Gnus under Emacs, which you'll like if an only if you like
Emacs.

[Malcolm McLean]

On Sunday, August 25, 2013 10:02:42 PM UTC+1, Keith Thompson wrote:
> Malcolm McLean <malcolm...@btinternet.com> writes:
>

> > That has to be balanced against the disadvantage of writing
> > code which is difficult for a human to read.
>
> No, it doesn't. Learn the idiom once, understand it forever.
>
> Do *you* find it difficult, or do you merely assume that everyone
> else does? (I've asked you this, in different words, several times;
> you have yet to answer.)
>

Hard to read. Not difficult intellectually of course.

one issue is

ptr = malloc(N * sizeof *ptr);

uses the same operator glyph in different contexts which aren't lexically
distinguished, except by an optional whitespace convention.

[gdo...@gmail.com]

actually i'm just cutting and pasting because i have noticed my typos of the pass.

style, i can certainly tighten things up and drop much of the comments to help
those that are use a tighter style read it. that copying and pasting maybe adding a little
extra. i checked my editor, i thought it was set to 80 chars, it was not, i changed it to
72. i believe the code is less than 80 chars in width. comments are long winded and longer.

hey, thanks i will repost. let know if it better.

[gdo...@gmail.com]

/* This program creates and uses pointers to pointer of type char,
pointer to arrays of char, malloc(), to allocate memory for
the pointer to pointer and 1-D arrays, strlen(), sizeof(),
and strtok(). Using those functions the program separates words,
in array of character arrays, and string constants. It places
those words in an allocated spaces. This program builds from
simple statements and ideas so not to overwhelm. */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main( void )
{
char **a;

int numberOfPointers = 5;

a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );

if ( a!= NULL )
printf( "the array, a, points to declare space\n\n" );

char *b[] = { "one", "two", "three", "four", "five" };

a = b;

for ( int index = 0; index < 5; index++ )
{
printf( "%s\n\n", *(a+index) );
}

for ( int index = 0; index < 5; index++ )
{
printf( "%s\n\n", *(a+index) );
}

char *string = "word" ;

size_t n = strlen( string );

printf("\n %zd \n\n", n); /* %zd because n is of type size_t */

char sentence[] = "this is a sentence with words";
char *token = " ";
char *g;

g = strtok(sentence, token);

size_t y = strlen(g);

printf("the length of the first word in the sentence is %d\n\n", (int) y );

char *c_space = (char *) malloc( y * sizeof(char) + 1 );

if (c_space)
{
printf("got space for word\n\n");
}

int indexnewstring = (int) y + 1; /* (int) strlen( g ) + 1 */

char newstring[indexnewstring];

char *v = strcpy( newstring, g);
printf(" %s\n", v );

char sentence2[] = "this is a sentence with words";
g = strtok( sentence2, token );

while ( g != NULL )
{
printf("\n %s", g );
g = strtok( NULL, token );
}

printf("\n");

char sentence3[] = "C programming actually is the fun";

for( g = strtok(sentence3, token ); g != NULL; g = strtok( NULL, token ) )
printf (" %s\n", g );

return 0;
}

Questions: How do you free a pointer to a pointer of what it points to?
How would you free the pointer of the pointer a. (a+index)?
strtok(), on my machine, seems to only take an array of characters. Is
the function suppose to string constants too.
sen4 = {"this is a short sentence"};

ken i hope this posting is better. let me know. thanks.

[gdo...@gmail.com]

> keith i hope this posting is better. let me know. thanks.

to free the memory of what a's pointers point to would it be:
free((void *) a+index);

thanks.

[Ike Naar]

On 2013-08-25, Keith Thompson <ks...@mib.org> wrote:
> James Kuyper <james...@verizon.net> writes:
>> On 08/24/2013 03:05 PM, Sharwan Joram wrote:
> [...]
>>> if ( NULL == parameters[parametercount]){
>
> This is what's known as a "Yoda conndition"
><http://en.wikipedia.org/wiki/Yoda_Conditions>. I know that a lot of
> programmers like them, and for somewhat valid reasons, but personally I
> find them jarring and unnecessary. Personally, I'd write that as:
>
> if (parameters[parametercount] == NULL) {

Does it matter? The == operator is symmetric, (X==Y) == (Y==X).

If (X==Y) is jarring and unnecessary, then, for symmetry reasons
(Y==X) is unnecessary and jarring.

"It is allowed on all hands, that the primitive way of breaking
eggs, before we eat them, was upon the larger end; but his present
majesty's grandfather, while he was a boy, going to eat an egg,
and breaking it according to the ancient practice, happened to cut
one of his fingers. Whereupon the emperor his father published an
edict, commanding all his subjects, upon great penalties, to break
the smaller end of their eggs. The people so highly resented this
law, that our histories tell us, there have been six rebellions
raised on that account; wherein one emperor lost his life, and
another his crown. These civil commotions were constantly fomented
by the monarchs of Blefuscu; and when they were quelled, the exiles
always fled for refuge to that empire. It is computed that eleven
thousand persons have at several times suffered death, rather than
submit to break their eggs at the smaller end"
(Jonathan Swift, Gulliver's Travels, part 1: A Voyage to Lilliput)

[gdo...@gmail.com]

/* this runs to end, does it do what you what?, stick some data in and see */
/* tried to stick with your main thoughts, variable, etc */

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, const char * argv[])
{

char **parameters;
int idx = 0;
int parameterCount = 5; /* initializing for the purposes of testing */

// char *saved_token; /* would suggest renaming this variable */
/* *saved_token must point to a string */
/* constant from somewhere. using a string */
/* const creates a memory error for my compiler */
/* Clang. but strtok() works fine with an */
/* char array */

char saved_token[] = "I was initialized by another function";

char *token; /* change from a char to char * */


parameters = (char **)malloc(parameterCount * sizeof(char *));
/* you should add your check using fprintf ... */


/* hopefully placing parameterCount in for header is not that big a */
/* for you */

parameterCount = 0;

/* i don't understand token && *token */
/* when are you trying to end the loop, at the end of the saved_token? */

for( token = strtok(saved_token, " "); token =='\0' ; token = strtok(NULL, " "))
{
parameters[parameterCount] = (char *)malloc(30 * sizeof (char *));
/* you should add your check using fprintf ... */

/* many have said just use strcpy(). well yeah it would copy the */
/* tokens in the space allocated */
/* calloc is nice too, but orignal code was not bad, in the sense */
/* of being poorly thought out. it was a good shot. */
/* the question is this the way you need to do it or just want to */
/* to do it */

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

/* adding idx to your for loop so it can keep count of your tokens */
idx++; /* idx will count the number of tokens */

}

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

for ( idx = 0; idx <= parameterCount; idx++ ) /* changed from orig */
{
free((void *)parameters+idx);
} /* braces added to draw attention only, not need of course */

free((void *) *parameters); /* not sure this frees what you want */

}

return 0;
}

[gdo...@gmail.com]

i made a logical i see in the for loop... token != '\0'
i don't think even changing it to this is logically correct...

[James Kuyper]

On 08/26/2013 02:40 AM, Ike Naar wrote:
> On 2013-08-25, Keith Thompson <ks...@mib.org> wrote:
>> James Kuyper <james...@verizon.net> writes:
>>> On 08/24/2013 03:05 PM, Sharwan Joram wrote:
>> [...]
>>>> if ( NULL == parameters[parametercount]){
>>
>> This is what's known as a "Yoda conndition"
>> <http://en.wikipedia.org/wiki/Yoda_Conditions>. I know that a lot of
>> programmers like them, and for somewhat valid reasons, but personally I
>> find them jarring and unnecessary. Personally, I'd write that as:
>>
>> if (parameters[parametercount] == NULL) {
>
> Does it matter? The == operator is symmetric, (X==Y) == (Y==X).
>
> If (X==Y) is jarring and unnecessary, then, for symmetry reasons
> (Y==X) is unnecessary and jarring.

If jarring and unnecessary is (X==Y), then for symmetry reasons,
unnecessary and jarring is (Y==X).
--
James Kuyper

[James Kuyper]

On 08/26/2013 02:07 AM, gdo...@gmail.com wrote:
> /* This program creates and uses pointers to pointer of type char,
> pointer to arrays of char, malloc(), to allocate memory for
> the pointer to pointer and 1-D arrays, strlen(), sizeof(),
> and strtok(). Using those functions the program separates words,
> in array of character arrays, and string constants. It places
> those words in an allocated spaces. This program builds from
> simple statements and ideas so not to overwhelm. */
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> int main( void )
> {
> char **a;
> int numberOfPointers = 5;
>
> a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );

The cast is unnecessary, and if you compile using C90, could mask an error.

> if ( a!= NULL )
> printf( "the array, a, points to declare space\n\n" );
>
> char *b[] = { "one", "two", "three", "four", "five" };
>
> a = b;
>
> for ( int index = 0; index < 5; index++ )
> {
> printf( "%s\n\n", *(a+index) );

*(a+index) is exactly equivalent to a[index]. It only saves two
characters in typing, but it also makes it easier to read.

> }
>
> for ( int index = 0; index < 5; index++ )
> {
> printf( "%s\n\n", *(a+index) );
> }
>
> char *string = "word" ;
>
> size_t n = strlen( string );
>
> printf("\n %zd \n\n", n); /* %zd because n is of type size_t */
>
> char sentence[] = "this is a sentence with words";
> char *token = " ";
> char *g;
>
> g = strtok(sentence, token);

The second argument of strtok is a list of delimiters that are be used
for breaking the string up into tokens. Naming that list "token"
suggests a misunderstanding of some kind (except for entries in the
Obfuscated C contest).

> size_t y = strlen(g);
>
> printf("the length of the first word in the sentence is %d\n\n", (int) y );
>
> char *c_space = (char *) malloc( y * sizeof(char) + 1 );

The cast is unnecessary in C. sizeof(char) is inherently 1, so that
should be just y+1.

> if (c_space)
> {
> printf("got space for word\n\n");
> }

You reserve space for the word, but then don't use it, and also don't
free() it, so you've got a memory leak. I presume this is related to
your question below, even though it involves neither 'a' nor 'index'.

> int indexnewstring = (int) y + 1; /* (int) strlen( g ) + 1 */

The cast is unnecessary. You should only use casts when they are
necessary. Any time you read or write a cast in a piece of C code, you
should examine it carefully, because necessary casts can do dangerous
things, things that often shouldn't be done. When you insert unnecessary
casts in your code, you make it harder to treat the necessary casts with
the proper level of caution.

> char newstring[indexnewstring];
>
> char *v = strcpy( newstring, g);
> printf(" %s\n", v );
>
> char sentence2[] = "this is a sentence with words";
> g = strtok( sentence2, token );
>
> while ( g != NULL )
> {
> printf("\n %s", g );
> g = strtok( NULL, token );
> }
>
> printf("\n");
>
> char sentence3[] = "C programming actually is the fun";
>
> for( g = strtok(sentence3, token ); g != NULL; g = strtok( NULL, token ) )
> printf (" %s\n", g );
>
> return 0;
> }
>
> Questions: How do you free a pointer to a pointer of what it points to?
> How would you free the pointer of the pointer a. (a+index)?

I'm not sure I understand your question, because the answer seems to be
too simple to cause any confusion, and refers to a case that comes up
nowhere in the above code. The rule of thumb is that if you have

expression = malloc(count * sizeof *expression)

Then it should be matched by:

free(expression);

Such matching pieces of code are sometime referred to metaphorically as
"bookend code". In the above code, you should somewhere have had

a[index] = malloc(count * sizeof *a[index]);

in which case the matching free should look like

free(a[index]);

Note that you should NOT free(a) until you've already called
free(a[index]) for every case where a[index] contains a pointer returned
by a call to malloc(). No such cases come up in your code above.

Does that answer your question? If not, could you explain the context of
the question more explicitly, preferably in the form of C code?

> strtok(), on my machine, seems to only take an array of characters. Is
> the function suppose to string constants too.
> sen4 = {"this is a short sentence"};

No. strtok() writes to the array that it is parsing. Writing to the
array pointed at by a string literal has undefined behavior, so you
should definitely NOT write such code.
--
James Kuyper

[Richard Damon]

Only if the jarring is based on pure C language semantics. I think the
previous person was referring to the fact that for English speakers, the
sentence X equals Y, has a different grammatical implications than Y
equals X, as the first variable is the subject of the verb equals and
the second is the object of the verb, and linguistically, the subject of
the verb is more important and should naturally be the variable, not the
constant, as the subject is what is being tested and the object the
value being tested for. It is just the fact that equals has this funny
transitive property that makes swapping make sense programatically, even
if it doesn't really change what makes sense linguistically.

The order NULL == var is jarring because we have no need to test for the
properties of NULL, we know everything possible about it.

[James Kuyper]

On 08/26/2013 02:40 AM, Ike Naar wrote:

> On 2013-08-25, Keith Thompson <ks...@mib.org> wrote:
>> James Kuyper <james...@verizon.net> writes:
>>> On 08/24/2013 03:05 PM, Sharwan Joram wrote:
>> [...]
>>>> if ( NULL == parameters[parametercount]){
>>
>> This is what's known as a "Yoda conndition"
>> <http://en.wikipedia.org/wiki/Yoda_Conditions>. I know that a lot of
>> programmers like them, and for somewhat valid reasons, but personally I
>> find them jarring and unnecessary. Personally, I'd write that as:
>>
>> if (parameters[parametercount] == NULL) {
>
> Does it matter? The == operator is symmetric, (X==Y) == (Y==X).

Yes, it matters - because it's intended to protect against the typo
which replaces == with =, by ensuring that it would cause a constraint
violation. The assignment operator is very definitely not symmetric.
--
James Kuyper

[Barry Schwarz]

On Sun, 25 Aug 2013 23:07:43 -0700 (PDT), gdo...@gmail.com wrote:

>/* This program creates and uses pointers to pointer of type char,
> pointer to arrays of char, malloc(), to allocate memory for
> the pointer to pointer and 1-D arrays, strlen(), sizeof(),
> and strtok(). Using those functions the program separates words,
> in array of character arrays, and string constants. It places
> those words in an allocated spaces. This program builds from
> simple statements and ideas so not to overwhelm. */
>
>#include <stdio.h>
>#include <stdlib.h>
>#include <string.h>
>
>int main( void )
>{
> char **a;
> int numberOfPointers = 5;
>
> a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );
>
> if ( a!= NULL )
> printf( "the array, a, points to declare space\n\n" );

a is not an array. It points to allocated memory.

>
> char *b[] = { "one", "two", "three", "four", "five" };
>
> a = b;

The allocated memory that a used to point to is now completely
inaccessible. It cannot be used nor can it be freed. This is a
classic memory leak.

If you had not destroyed the value of a as noted above, you would
simply
free(a);

> How would you free the pointer of the pointer a. (a+index)?

(a+index) is simply an address somewhere within whatever a points to.
It cannot be freed.

On the other hand, if you meant *(a+index), then:

As your code stands, a points to b. (a+index) is the address of
b[index]. Therefore, *(a+index) is an alias for the value b[index].
Since b[index] is a pointer that points to a string literal, it cannot
be freed. The only values that can be passed to free are NULL and the
address returned from malloc/calloc/realloc.

If your intent was to allocate space for an array of pointers and
then to allocate space for each of those pointers so they pointed to
strings, your code would look something like
char **a = malloc(...); /* allocate space for pointers */
for (i = 0; i < N; i++)
a[i] = malloc(...); /* allocate space for strings */
/* obtain data for the k-th string */
strcpy(a[k], g); /* copy string to allocated space */
When it was time to free the space occupied by the strings and the
pointers, your code would look something like
for (i = 0; i < N; i++)
free(a[i]); /*free each individual string */
free(a); /* free array of pointers */
Of course, you would do error checking on each of the calls to malloc.

> strtok(), on my machine, seems to only take an array of characters. Is
> the function suppose to string constants too.

Since strtok changes the string it points to (by inserting the '\0'
character in place of delimiter that marks the end of a token), it
cannot be used on a string literal. Any attempt to modify a string
literal invokes undefined behavior.

> sen4 = {"this is a short sentence"};

Assuming you meant char *sentence4 = {...}, then the data sentence4
points to cannot be parsed by strtok.

--
Remove del for email

[Ben Bacarisse]

gdo...@gmail.com writes:
<snip>

> char **a;
> int numberOfPointers = 5;
>
> a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );
>
> if ( a!= NULL )
> printf( "the array, a, points to declare space\n\n" );
>
> char *b[] = { "one", "two", "three", "four", "five" };
>
> a = b;

It's possible that you don't know what this assignment does. It only
replaces one pointer with another. The pointer value that used to be in
'a' is lost so you can no longer access the memory that was allocated.
in particular, you can't ever free it now.

<snip>
--
Ben.

[Ben Bacarisse]

gdo...@gmail.com writes:

> On Monday, August 26, 2013 2:07:43 AM UTC-4, gdo...@gmail.com wrote:
>> char **a;
>> int numberOfPointers = 5;
>>
>> a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );

<snip>

>> char *b[] = { "one", "two", "three", "four", "five" };
>> a = b;

<snip>

> to free the memory of what a's pointers point to would it be:
> free((void *) a+index);

If 'a' did in fact point to (the start of) an array of pointers that
could be freed you'd write

free(a[index]);

If you try that with your program, it won't work because, while 'a' does
pointer to an array of pointers, they are not "freeable". a[0], a[1]
etc all point to statically allocated arrays of chars. These arrays get
made when you use string literals like "one" and "two". You can't free them.

--
Ben.

[Sharwan Joram]

Hi,
It was fixed when i realized that I have not given the proper space in allocation of the double indirection pointer @ :

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

to
parameters = malloc((parametercount + 1) * sizeof(char *));
.
I learned lots of new stuff from you all guys during this discussion.

Thanks everyone for the help !!

[David Brown]

The "Yoda condition" is the habit of writing "if (1 == a)" rather than
"if (a == 1)", so that typos will lead to compiler errors - "if (1 = a)"
is a syntax error, while "if (a = 1)" is legal C and can be accepted by
the compiler.

However, that assumes you either have a very poor compiler (and no lint
tool to back it up), or you don't know how to use it properly. Unless
you are masochistic and enjoy finding hard-to-debug errors in your code,
you let your compiler do a lot of static error checking. For example,
with gcc you would normally enable at least "-Wall" warnings. Then "if
(a = 1)" will give you a warning, as your code is probably wrong, and
you must write "if ((a = 1))" to disable that warning. Compilers will
often also give you warnings about unreachable code, since the
conditional always goes the same way.

In my opinion, it is better to do proper static error checking to
eliminate the risk of such typos, rather than to write your code backwards.

[Ben Bacarisse]

Sharwan Joram <sharwa...@gmail.com> writes:

> On Monday, August 26, 2013 5:28:47 PM UTC+5:30, Ben Bacarisse wrote:
>> gdo...@gmail.com writes:
>> <snip>
>> > char **a;
>> > int numberOfPointers = 5;
>> >
>> > a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );
>> >
>> > if ( a!= NULL )
>> > printf( "the array, a, points to declare space\n\n" );
>> >
>> > char *b[] = { "one", "two", "three", "four", "five" };
>> >
>> > a = b;
>>
>> It's possible that you don't know what this assignment does. It only
>> replaces one pointer with another. The pointer value that used to be in
>> 'a' is lost so you can no longer access the memory that was allocated.
>> in particular, you can't ever free it now.
>>

> It was fixed when i realized that I have not given the proper
> space in allocation of the double indirection pointer @ :
>
> parameters = malloc(parametercount * sizeof(char *));
>
> to
> parameters = malloc((parametercount + 1) * sizeof(char *));

I think you are confused. You did have a problem allocating too few
pointers, but in that case parametercount was simply misnamed (or
misused) -- you'd counted delimiters between parameters, not parameters
themselves.

Anyway, none of this has anything to do with my remark above.

<snip>
--
Ben.

[Ike Naar]

On 2013-08-26, Richard Damon <Ric...@Damon-Family.org> wrote:
> Only if the jarring is based on pure C language semantics. I think the
> previous person was referring to the fact that for English speakers, the
> sentence X equals Y, has a different grammatical implications than Y
> equals X, as the first variable is the subject of the verb equals and
> the second is the object of the verb, and linguistically, the subject of
> the verb is more important and should naturally be the variable, not the
> constant, as the subject is what is being tested and the object the
> value being tested for. It is just the fact that equals has this funny
> transitive property that makes swapping make sense programatically, even
> if it doesn't really change what makes sense linguistically.
>
> The order NULL == var is jarring because we have no need to test for the
> properties of NULL, we know everything possible about it.

A mathematical formula is not English prose and should not be read as such.

[gdo...@gmail.com]

yes it is equivalent.

>
>
>
> > }
>
> >
>
> > for ( int index = 0; index < 5; index++ )
>
> > {
>
> > printf( "%s\n\n", *(a+index) );
>
> > }
>
> >
>
> > char *string = "word" ;
>
> >
>
> > size_t n = strlen( string );
>
> >
>
> > printf("\n %zd \n\n", n); /* %zd because n is of type size_t */
>
> >
>
> > char sentence[] = "this is a sentence with words";
>
> > char *token = " ";
>
> > char *g;
>
> >
>
> > g = strtok(sentence, token);
>
>
>
> The second argument of strtok is a list of delimiters that are be used
>
> for breaking the string up into tokens. Naming that list "token"
>
> suggests a misunderstanding of some kind (except for entries in the
>
> Obfuscated C contest).

ok, i see your point, next time i will use delimiter

>
>
>
> > size_t y = strlen(g);
>
> >
>
> > printf("the length of the first word in the sentence is %d\n\n", (int) y );
>
> >
>
> > char *c_space = (char *) malloc( y * sizeof(char) + 1 );
>
>
>
> The cast is unnecessary in C. sizeof(char) is inherently 1, so that
>
> should be just y+1.
>
>

didn't know that.

>
> > if (c_space)
>
> > {
>
> > printf("got space for word\n\n");
>
> > }
>
>
>
> You reserve space for the word, but then don't use it, and also don't
>
> free() it, so you've got a memory leak. I presume this is related to
>
> your question below, even though it involves neither 'a' nor 'index'.
>
>
>
> > int indexnewstring = (int) y + 1; /* (int) strlen( g ) + 1 */
>
>

no, not really.

ok

>
> Such matching pieces of code are sometime referred to metaphorically as
>
> "bookend code". In the above code, you should somewhere have had
>
>
>
> a[index] = malloc(count * sizeof *a[index]);
>
>
>
> in which case the matching free should look like
>
>
>
> free(a[index]);
>
>
>
> Note that you should NOT free(a) until you've already called
>
> free(a[index]) for every case where a[index] contains a pointer returned
>
> by a call to malloc(). No such cases come up in your code above.
>
>
>
> Does that answer your question? If not, could you explain the context of
>
> the question more explicitly, preferably in the form of C code?
>
>
>
> > strtok(), on my machine, seems to only take an array of characters. Is
>
> > the function suppose to string constants too.
>
> > sen4 = {"this is a short sentence"};
>
>
>
> No. strtok() writes to the array that it is parsing. Writing to the
>
> array pointed at by a string literal has undefined behavior, so you
>
> should definitely NOT write such code.

strtok() writes to an array?


>
> --
>
> James Kuyper

thanks

[Keith Thompson]

gdo...@gmail.com writes:
> On Sunday, August 25, 2013 11:01:30 PM UTC-4, Keith Thompson wrote:
>> gdo...@gmail.com writes:
>>
>> [huge snip]
>>
>>
>>
>> Your articles are difficult to read, partly because you're using the
>>
>> broken Google Groups interface to Usenet.
>>
>>
>>
>> GG encourages (or at least fails to discourage) very long lines, and it
>>
>> handles quoted text poorly. Due, I think, to an assumption that
>>
>> newlines denote paragraph breaks rather than line breaks, quoted text
>>
>> tends to be double-spaced -- and quoted quoted text tends to be
>>
>> quadruple-spaced, and so on.

[...]

> actually i'm just cutting and pasting because i have noticed my typos of the pass.
>
> style, i can certainly tighten things up and drop much of the comments to help
> those that are use a tighter style read it. that copying and pasting maybe adding a little
> extra. i checked my editor, i thought it was set to 80 chars, it was not, i changed it to
> 72. i believe the code is less than 80 chars in width. comments are long winded and longer.
>
> hey, thanks i will repost. let know if it better.

Your quoted text is still double-spaced; see above.

This is the fault of the broken Google Groups interface (years of
complaints have not prompted Google to fix this), but you should be
able to avoid the problem. Either delete the blank lines yourself,
or use an Usenet interface other than Google Groups.

[James Kuyper]

On 08/26/2013 10:08 AM, David Brown wrote:
...

> The "Yoda condition" is the habit of writing "if (1 == a)" rather than
> "if (a == 1)", so that typos will lead to compiler errors - "if (1 = a)"
> is a syntax error, while "if (a = 1)" is legal C and can be accepted by
> the compiler.
>
> However, that assumes you either have a very poor compiler (and no lint
> tool to back it up), or you don't know how to use it properly. Unless
> you are masochistic and enjoy finding hard-to-debug errors in your code,
> you let your compiler do a lot of static error checking.

I don't use Yoda-conditions, but I appreciate the arguments of those
that do. I don't like to rely on diagnostics that are not mandated by
the standard, and the ones you're talking about aren't. I do prefer
compilers that provide such warnings.

...

> In my opinion, it is better to do proper static error checking to
> eliminate the risk of such typos, rather than to write your code backwards.

The code, as such, is neither forwards nor backwards. The idea that
there's a preferred order to the arguments of == has nothing to do with
any aspect of C itself; it seems to be based in linguistic preferences.
I share those preferences, but I wouldn't elevate them to issues of code
correctness, by calling the other order "backwards". I wouldn't be
surprised to find that people whose native language is not English might
find the "backwards" order more reasonable.

[gdo...@gmail.com]

i don't see that on my end. your post are double spaced.

[James Kuyper]

On 08/26/2013 11:27 AM, gdo...@gmail.com wrote:
> On Monday, August 26, 2013 7:02:31 AM UTC-4, James Kuyper wrote:
>> On 08/26/2013 02:07 AM, gdo...@gmail.com wrote:

...

>>> strtok(), on my machine, seems to only take an array of characters. Is
>>> the function suppose to string constants too.
>>> sen4 = {"this is a short sentence"};
>>
>> No. strtok() writes to the array that it is parsing. Writing to the
>> array pointed at by a string literal has undefined behavior, so you
>> should definitely NOT write such code.
>
>
> strtok() writes to an array?

Yes, it's first argument is an input/output pointer. It replaces the
first delimiter after each token in the input string with a '\0'.

char sen5[] = "this is a short sentence";

Calling strtok(sen5, " ") would set sen5[4] to '\0'. If your code then
calls strtok(NULL, " "), it would set sen5[7] to '\0'. If you called
strtok("this is a short sentence", " "), it would attempt to do the same
thing, but attempting to write to the array pointed at by a string
literal would render the behavior of your program undefined.

[Keith Thompson]

Ike Naar <i...@iceland.freeshell.org> writes:
> On 2013-08-25, Keith Thompson <ks...@mib.org> wrote:
>> James Kuyper <james...@verizon.net> writes:
>>> On 08/24/2013 03:05 PM, Sharwan Joram wrote:
>> [...]
>>>> if ( NULL == parameters[parametercount]){
>>
>> This is what's known as a "Yoda conndition"
>><http://en.wikipedia.org/wiki/Yoda_Conditions>. I know that a lot of
>> programmers like them, and for somewhat valid reasons, but personally I
>> find them jarring and unnecessary. Personally, I'd write that as:
>>
>> if (parameters[parametercount] == NULL) {
>
> Does it matter? The == operator is symmetric, (X==Y) == (Y==X).
>
> If (X==Y) is jarring and unnecessary, then, for symmetry reasons
> (Y==X) is unnecessary and jarring.

[...]

Yes, it matters *to me* (and to plenty of other people). "==" is
commutative as far as the language is concerned, but code should be
written both for the compiler and for the human reader.

X==Y is a poor example for this, since X==Y and Y==X are pretty much
equally readable. The problem (for me) is when programmers write things
like:
if (0 == strcmp(s1, s2))
rather than
if (strcmp(s1, s2) == 0)

When comparing a computed value to a constant, I'm asking a question
about the computed value: (Is it equal to this constant?) You can
ask a question about a constant: (Is it equal to this computed
value?), but I personally find that to be an awkward way to ask
the same question.

If you find (strcmp(s1, s2) == 0) just as readable and natural as
(0 == strcmp(s1, s2)), that's great for you; apparently your brain
works just a little bit differently than mine.

If your only criterion for choosing between two different ways
of writing something is that they have the same language-level
semantics, that should mean you have no preference between arr[i]
and i[arr], or between

if (foo) {
/* ... */
}

and

if (!foo); else {
/* ... */
}

but I know which I'd rather see.

I know why Yoda conditions are used, and it's a valid reason.
But would you even consider writing

if (0 == strcmp(s1, s2))

if it weren't for the "==" vs. "=" issue?

[Keith Thompson]

gdo...@gmail.com writes:
[...]

> strtok() writes to an array?

Yes. You should know this if you're read the documentation for strtok,
and you should never use *any* function unless you've read its
documentation. Eventually you'll become sufficiently familiar with some
functions that you don't need always to do this, but if you have a
question about how a function (like "strtok() writes to an array?"),
your first stop should be the documentation.

[Keith Thompson]

gdo...@gmail.com writes:
> On Monday, August 26, 2013 11:29:56 AM UTC-4, Keith Thompson wrote:
>> gdo...@gmail.com writes:

[...]

>> Your quoted text is still double-spaced; see above.
>>
>>
>>
>> This is the fault of the broken Google Groups interface (years of
>>
>> complaints have not prompted Google to fix this), but you should be
>>
>> able to avoid the problem. Either delete the blank lines yourself,
>>
>> or use an Usenet interface other than Google Groups.
>

> i don't see that on my end. your post are double spaced.

My post was not double spaced. Your quotation of my post is.

As an experiment, I started to compose a followup in Google Groups.
I could clearly see that the double-spaced quoted text, and I was
able to clean it up by copy-and-pastiung the text into an editor,
fixing it, and copy-and-pasting it back.

[David Brown]

On 26/08/13 17:42, James Kuyper wrote:
> On 08/26/2013 10:08 AM, David Brown wrote:
> ...
>> The "Yoda condition" is the habit of writing "if (1 == a)" rather than
>> "if (a == 1)", so that typos will lead to compiler errors - "if (1 = a)"
>> is a syntax error, while "if (a = 1)" is legal C and can be accepted by
>> the compiler.
>>
>> However, that assumes you either have a very poor compiler (and no lint
>> tool to back it up), or you don't know how to use it properly. Unless
>> you are masochistic and enjoy finding hard-to-debug errors in your code,
>> you let your compiler do a lot of static error checking.
>
> I don't use Yoda-conditions, but I appreciate the arguments of those
> that do. I don't like to rely on diagnostics that are not mandated by
> the standard, and the ones you're talking about aren't. I do prefer
> compilers that provide such warnings.
>

C allows far too much freedom to write incorrect (or technically
correct, but unintelligible) code. If a student was doing a programming
language design course and submitted C as a project, they'd get an
immediate fail. But for many reasons, it is the best language we have
for a lot of uses, so we use it despite its failings. However, I think
it is very important to limit the risk of mistakes, and to improve the
readability and safety of the language by using good programming styles
and good programming tools (such as static error checking). These don't
limit or detract from the good points of C, but make it into a more
complete and modern development tool. In fact, as I understand my
history, part of the reason that C was designed so "tolerant" of errors
is that it was assumed that programmers would use "lint" tools -
separating the actual compilation from the static error checking. So
for my own use, and for the use of others at my work, I don't see such
warning flags and error checking as a "nice to have" extra - I see them
as mandatory for good development practice. But I am fully aware that
others have different opinions and practices.

> ...
>> In my opinion, it is better to do proper static error checking to
>> eliminate the risk of such typos, rather than to write your code backwards.
>
> The code, as such, is neither forwards nor backwards. The idea that
> there's a preferred order to the arguments of == has nothing to do with
> any aspect of C itself; it seems to be based in linguistic preferences.
> I share those preferences, but I wouldn't elevate them to issues of code
> correctness, by calling the other order "backwards". I wouldn't be
> surprised to find that people whose native language is not English might
> find the "backwards" order more reasonable.
>

I understand your arguments here. But I program in English, as do most
people whether they are native speakers or not (I live and work in
Norway, so I use Norwegian for most non-programming activities). Like
most programming languages, C was designed by English speakers without
much consideration for other languages - and I therefore don't see any
issues with tying the concept of "backwards" here to the English language.

A vital part of good programming is writing code that reads easily and
clearly. I think that "if (a == 1)" makes more sense when read than "if
(1 == a)" - therefore I think it is better code.

But as noted above, this is all my opinion. I can dictate this sort of
thing within my company, but outside that I have to accept that some
people have different thoughts, even if they are wrong :-)

[James Kuyper]

On 08/26/2013 11:46 AM, gdo...@gmail.com wrote:
> On Monday, August 26, 2013 11:29:56 AM UTC-4, Keith Thompson wrote:

...

>> This is the fault of the broken Google Groups interface (years of
>>
>> complaints have not prompted Google to fix this), but you should be
>>
>> able to avoid the problem. Either delete the blank lines yourself,
>>
>> or use an Usenet interface other than Google Groups.

...

> i don't see that on my end. your post are double spaced.

When I look at this thread using Google Groups:

The original message from Sharwan Joram,
<https://groups.google.com/d/msg/comp.lang.c/jtP__M9Xd9o/fW2zlPTbxD4J>,
is single spaced.

The response by gdotone,
<https://groups.google.com/d/msg/comp.lang.c/jtP__M9Xd9o/r-XrYY2IueYJ>
contains text quoted from the previous message, which is double-spaced.

The response by Keith Thompson,
<https://groups.google.com/d/msg/comp.lang.c/jtP__M9Xd9o/8gkzz3A1GtIJ>
is single spaced.

The response by gdotone,
<https://groups.google.com/d/msg/comp.lang.c/jtP__M9Xd9o/JZCt8zLOCgkJ>
contains text quoted from Keith Thompson, which is double spaced.

The response by Keith Thompson,
<https://groups.google.com/d/msg/comp.lang.c/jtP__M9Xd9o/LiTdf7b55DsJ>
contains the text from Keith's previous message that was quoted by
gdotone, retaining the double-spacing that was introduced by gdotone's
message.

The response by gdotone,
<https://groups.google.com/d/msg/comp.lang.c/jtP__M9Xd9o/LgP6PBT6BzsJ?
contains text quoted from Keith's 2nd message. Since that message
contains double-spaced text quoted from Keith's first message, it is now
quadruple-spaced.

Do you see any double-spacing in any of those messages other than what
I've described above? Notice that in every case, your use of Google
Groups to respond with quoted text was the source of the double-spacing.
If you must use Google Groups, please remove the spurious extra space it
introduces; but you'd be better off using the alternatives he mentions.

[gdo...@gmail.com]

wait, i'm confused.

i am using googles groups web interface.
you are telling me, that when i post it is double spacing, because of something in GG's interface. ok.

but, when i copy an paste into it. it looks close to what i typed in the Xcode editor. when i look at my
postings, after summiting, there is no odd spacing.
when i reply to a post the text that i see is double space with arrows.
if you don't mind send me a screen shot of what you are seeing.

[gdo...@gmail.com]

On Monday, August 26, 2013 12:18:30 PM UTC-4, James Kuyper wrote:
> On 08/26/2013 11:46 AM, gdo...@gmail.com wrote:
> > On Monday, August 26, 2013 11:29:56 AM UTC-4, Keith Thompson wrote:

ok, i see what you are saying, writing.

[glen herrmannsfeldt]

James Kuyper <james...@verizon.net> wrote:
> On 08/26/2013 02:40 AM, Ike Naar wrote:

(snip)

>> Does it matter? The == operator is symmetric, (X==Y) == (Y==X).

> Yes, it matters - because it's intended to protect against the typo
> which replaces == with =, by ensuring that it would cause a constraint
> violation. The assignment operator is very definitely not symmetric.

Well, don't do that.

There are an infinite number of ways you can code C incorrectly,
and this is one.

There is a general rule that you should write for readability,
and as common mathematical English phrasing normally doesn't
work that way, it is more difficult to read.

Now, Java came up with another solution to this problem.
The expression in if must have type boolean, and, except in
the case of assignment to a boolean variable, and assignment
won't be boolean, the assignment won't compile.

It isn't possible to change C now, but to me readability is
important, and in many cases it sounds wrong backwards.

-- glen

[glen herrmannsfeldt]

James Kuyper <james...@verizon.net> wrote:
> On 08/26/2013 10:08 AM, David Brown wrote:

>> The "Yoda condition" is the habit of writing "if (1 == a)" rather than
>> "if (a == 1)", so that typos will lead to compiler errors - "if (1 = a)"

(snip)

Yes, I was just thinking about Yoda.

(snip)

> The code, as such, is neither forwards nor backwards. The idea that
> there's a preferred order to the arguments of == has nothing to do with
> any aspect of C itself; it seems to be based in linguistic preferences.
> I share those preferences, but I wouldn't elevate them to issues of code
> correctness, by calling the other order "backwards". I wouldn't be
> surprised to find that people whose native language is not English might
> find the "backwards" order more reasonable.

Reminds me of wondering why so many programming languages use
English keywords.

Seems to me that there might be some programming languages that do
assignment left to right, though most do it right to left.

It seems that both orders are used for assmeblers (and often
the corresponding machine instructions) for MOVE and other
data movement instructions.

So, yes, I would not be surprised to see a different order for
non-native English speakers, or non-English speakers coding C,
or other programming languages.

-- glen

[glen herrmannsfeldt]

Keith Thompson <ks...@mib.org> wrote:

(big snip)

> I know why Yoda conditions are used, and it's a valid reason.
> But would you even consider writing

> if (0 == strcmp(s1, s2))

> if it weren't for the "==" vs. "=" issue?

No, I would write

if( ! strcmp(s1, s2))

Talking about symmetry in operations, it still bothers me to write

if(s1.equals(s2))

in Java, which seems to make s1 and s2 very unequal. The symmetry
of the == operator is lost.

-- glen

[James Kuyper]

On 08/26/2013 12:54 PM, glen herrmannsfeldt wrote:
> James Kuyper <james...@verizon.net> wrote:
>> On 08/26/2013 02:40 AM, Ike Naar wrote:
>
> (snip)
>
>>> Does it matter? The == operator is symmetric, (X==Y) == (Y==X).
>
>> Yes, it matters - because it's intended to protect against the typo
>> which replaces == with =, by ensuring that it would cause a constraint
>> violation. The assignment operator is very definitely not symmetric.
>
> Well, don't do that.

If you know how to eliminate (not just reduce) errors created when
writing C (or anything else, for that matter), I'd appreciate hearing
about how it was done. If you have time for that - the secret to doing
something like that could make you very rich, if handled properly.

I don't know any such secret - so instead I use methods designed to
minimize the number of mistakes I make (which don't work very well). The
single most effective such method I've developed is to always write
bookend code (for instance, '(' and ')', or '{' and '}', or 'fopen()'
and 'fclose()') first, and then fill in what happens between the
bookends second. I almost never make "missing bookend" errors anymore.

I also use methods to ensure that most of mistakes I actually make get
caught (which works significantly better, though not as well as I'd
like). Yoda conditionals are one such method, though not one I've
decided to adopt. I agree that I find it uncomfortable - but I'd not
criticize anyone for feeling differently about that.

[Keith Thompson]

gdo...@gmail.com writes:
[...]

> wait, i'm confused.
>
> i am using googles groups web interface. you are telling me, that
> when i post it is double spacing, because of something in GG's
> interface. ok.

Yes.

> but, when i copy an paste into it. it looks close to what i typed in
> the Xcode editor. when i look at my postings, after summiting, there
> is no odd spacing. when i reply to a post the text that i see is
> double space with arrows. if you don't mind send me a screen shot of
> what you are seeing.

Your original text is not double spaced. Any text you quote from other
articles is. It shouldn't be; that's a bug / misfeature in Google
Groups.

Not a screenshot, but a plain text copy of your article saved from my
newsreader:

https://gist.github.com/Keith-S-Thompson/6344333

Usenet works best with plain text with lines, no longer than 72 or 80
columns, delimited by explicit end-of-line characters.

Google Groups seems to assume plain text with line breaks marking
paragraph boundaries.

The best solution is to stop posting through Google Groups. The
second-best solution is to fix the damage GG does before posting.

[Keith Thompson]

glen herrmannsfeldt <g...@ugcs.caltech.edu> writes:
> Keith Thompson <ks...@mib.org> wrote:
>
> (big snip)
>
>> I know why Yoda conditions are used, and it's a valid reason.
>> But would you even consider writing
>
>> if (0 == strcmp(s1, s2))
>
>> if it weren't for the "==" vs. "=" issue?
>
> No, I would write
>
> if( ! strcmp(s1, s2))

Again, this is a matter of personal taste.

I know what !strcmp(s1, s2) means, but I dislike it.

Many functions return a logically Boolean result, even if the result
isn't of type bool or _Bool. By "logically Boolean", I mean that
the result answers a yes/no question without adding more information.

strcmp is not one of those functions, so I dislike using its result
either as a condition or as the operand of "!". For much the same
reason, I dislike writing "if (ptr)" and strongly prefer "if (ptr != NULL)".

But of course that wasn't the point of the question. Using a different
example, would you even consider writing

if (5 == strlen(s))

rather than

if (strlen(s) == 5)

if it weren't for the "==" vs. "=" issue?

My point is that, as far as I can see, there is no reason to put a
constant expression on the LHS of an "==" comparison and a non-constant
expression on the RHS other than the attempt to avoid a potential "=="
vs. "=" error. And *in my opinion* that's not a good enough reason to
do it.

And if both forms are equally clear to you, that's great -- but be aware
that not everyone sees it that way.

> Talking about symmetry in operations, it still bothers me to write
>
> if(s1.equals(s2))
>
> in Java, which seems to make s1 and s2 very unequal. The symmetry
> of the == operator is lost.

I haven't done enough Java to comment on that, but at first glance I
agree that (s1.equals(s2)) isn't very pretty.

[Malcolm McLean]

On Monday, August 26, 2013 6:59:43 PM UTC+1, Keith Thompson wrote:

> glen herrmannsfeldt <g...@ugcs.caltech.edu> writes:
>
> > Talking about symmetry in operations, it still bothers me to write
>
> > if(s1.equals(s2))
>
> > in Java, which seems to make s1 and s2 very unequal. The symmetry
> > of the == operator is lost.
>
>
> I haven't done enough Java to comment on that, but at first glance I
> agree that (s1.equals(s2)) isn't very pretty.
>
>

Virtually all object languages have that problems. The syntax makes it look
like the functions operate on one object, taking the other as an argument.
Often that doesn't express very well what the code is doing.
In C++ you can overload the == operator, but operator overloadign creates its
own issues.

[Ike Naar]

On 2013-08-26, Keith Thompson <ks...@mib.org> wrote:
> Ike Naar <i...@iceland.freeshell.org> writes:
>> On 2013-08-25, Keith Thompson <ks...@mib.org> wrote:
>>> James Kuyper <james...@verizon.net> writes:
>>>> On 08/24/2013 03:05 PM, Sharwan Joram wrote:
>>> [...]
>>>>> if ( NULL == parameters[parametercount]){
>>>
>>> This is what's known as a "Yoda conndition"
>>><http://en.wikipedia.org/wiki/Yoda_Conditions>. I know that a lot of
>>> programmers like them, and for somewhat valid reasons, but personally I
>>> find them jarring and unnecessary. Personally, I'd write that as:
>>>
>>> if (parameters[parametercount] == NULL) {
>>
>> Does it matter? The == operator is symmetric, (X==Y) == (Y==X).
>>
>> If (X==Y) is jarring and unnecessary, then, for symmetry reasons
>> (Y==X) is unnecessary and jarring.
> [...]
>
> Yes, it matters *to me* (and to plenty of other people). "==" is
> commutative as far as the language is concerned, but code should be
> written both for the compiler and for the human reader.
>
> X==Y is a poor example for this, since X==Y and Y==X are pretty much
> equally readable.

X and Y were meant to be placeholders for arbitrary subexpressions.

I prefer to treat (X==Y) as a mathematical formula (stating that
subjects X and Y have equal values), rather than shoehorn it into
an English sentence that has X, but not Y, as its subject.

How about Yodaness when both operands are non-constant, as in
(sin(alpha) == cos(beta)) ?.
Would it now matter which operand goes on the left side?

What if it's unknown which of the operands is constant?
Consider the expression (pNeedle == pHaystack).
If we assume that pNeedle is variable and pHaystack is
constant, this expression is, supposedly, perfectly readable.
But if it turns out that pNeedle is constant and pHaystack is
variable, the same expression suddenly becomes Yoda? So Yodaness
is not a syntactic property of the expression itself, but also
depends on additional knowledge that may or may not be obvious from
looking at the expression and its context, and that even may change
at runtime?

I think the main purpose of the == operator is to test whether
two values are equal. For that purpose it does not really matter
whether operands are constant or not: it's equality we're
interested in, not constness. Taking constness into consideration
just complicates things unnecessarily; so let that not influence
the way equalities are notated.

By the way, does Yodaness apply to other commutative operators as
well? Which one of (2 * f(x)) and (f(x) * 2) is Yoda, and why?

> The problem (for me) is when programmers write things
> like:
> if (0 == strcmp(s1, s2))
> rather than
> if (strcmp(s1, s2) == 0)
>
> When comparing a computed value to a constant, I'm asking a question
> about the computed value: (Is it equal to this constant?) You can
> ask a question about a constant: (Is it equal to this computed
> value?), but I personally find that to be an awkward way to ask
> the same question.

It might help to re-phrase the question to one that is more symmetric
in its operands, e.g. "are ... and ... equal?".

> If you find (strcmp(s1, s2) == 0) just as readable and natural as
> (0 == strcmp(s1, s2)), that's great for you; apparently your brain
> works just a little bit differently than mine.
>
> If your only criterion for choosing between two different ways
> of writing something is that they have the same language-level
> semantics, that should mean you have no preference between arr[i]
> and i[arr],

In this case I'd prefer arr[i]. There is a longstanding tradition
in programming languages to write arr[i]. C allows i[arr], but other
programming languages don't. In mathematics notation one sometimes
writes 'arr' with a subscripted 'i' to the right, but I've never
seen that the other way around.

In contrast, the equality operator is symmetric both in mathematics
and in most (every?) programming language that I know of.

> or between
> if (foo) {
> /* ... */
> }
>
> and
>
> if (!foo); else {
> /* ... */
> }
>
> but I know which I'd rather see.

I'd prefer the first form because it's simpler.

> I know why Yoda conditions are used, and it's a valid reason.
> But would you even consider writing
>
> if (0 == strcmp(s1, s2))
>
> if it weren't for the "==" vs. "=" issue?

Actually the Yoda form can be slightly more readable when the
function call has a long argument list, as in

if (3 == scanf("%g %g %g", &latitude, &longitude, &elevation))

one can immediately see that the returnvalue of scanf is compared to 3,
because the 3 is close to the scanf, whereas in

if (scanf("%g %g %g", &latitude, &longitude, &elevation) == 3)

the 3 and the scanf are far apart.

[glen herrmannsfeldt]

Keith Thompson <ks...@mib.org> wrote:

(snip)

>>> I know why Yoda conditions are used, and it's a valid reason.
>>> But would you even consider writing

>>> if (0 == strcmp(s1, s2))

>>> if it weren't for the "==" vs. "=" issue?

(snip, then I wrote)

>> No, I would write

>> if( ! strcmp(s1, s2))

> Again, this is a matter of personal taste.

> I know what !strcmp(s1, s2) means, but I dislike it.

I suppose I don't especially like it, but I usually do it anyway.

> Many functions return a logically Boolean result, even if the result
> isn't of type bool or _Bool. By "logically Boolean", I mean that
> the result answers a yes/no question without adding more information.

Yes, but == and != don't help all that much. The relation is still
backwards, but you get used to it. About as easy to get used to
with ! as with !=.

> strcmp is not one of those functions, so I dislike using its result
> either as a condition or as the operand of "!". For much the same
> reason, I dislike writing "if (ptr)" and strongly prefer
> "if (ptr != NULL)".

In this case, I think of the pointer as pointing to something, or not,
and so, to me, it makes some sense as a boolean.

> But of course that wasn't the point of the question. Using
> a different example, would you even consider writing

> if (5 == strlen(s))

> rather than

> if (strlen(s) == 5)

> if it weren't for the "==" vs. "=" issue?

> My point is that, as far as I can see, there is no reason to put a
> constant expression on the LHS of an "==" comparison and a non-constant
> expression on the RHS other than the attempt to avoid a potential "=="
> vs. "=" error. And *in my opinion* that's not a good enough reason to
> do it.

Yes, I don't like the

if (5 == strlen(s))

form. It looks ugly, and distracts me from reading the code.

But also I rarely do assignments in if statements.

> And if both forms are equally clear to you, that's great -- but be aware
> that not everyone sees it that way.

>> Talking about symmetry in operations, it still bothers me to write

>> if(s1.equals(s2))

>> in Java, which seems to make s1 and s2 very unequal. The symmetry
>> of the == operator is lost.

> I haven't done enough Java to comment on that, but at
> first glance I agree that (s1.equals(s2)) isn't very pretty.

There is also:

if("abc".equals(s1))

instead of

if(s1.equals("abc"))

which I never do, but supposedly has some advantage if s1
might be null.

-- glen

[glen herrmannsfeldt]

Malcolm McLean <malcolm...@btinternet.com> wrote:

(snip, I wrote)

>> > Talking about symmetry in operations, it still bothers me to write

>> > if(s1.equals(s2))

>> > in Java, which seems to make s1 and s2 very unequal. The symmetry
>> > of the == operator is lost.

> Virtually all object languages have that problems. The syntax
> makes it look like the functions operate on one object,
> taking the other as an argument. Often that doesn't express
> very well what the code is doing.

> In C++ you can overload the == operator, but operator
> overloadign creates its own issues.

I don't know C++ quite as well, but in Java you use == to compare
the reference, .equals() to compare the object. As Java doesn't
have operator overload, you couldn't do that, but you wouldn't
want to do it anyway. Among others, you couldn't test for null.

-- glen

[James Kuyper]

On 08/26/2013 03:18 PM, Ike Naar wrote:
...

> How about Yodaness when both operands are non-constant, as in
> (sin(alpha) == cos(beta)) ?.
> Would it now matter which operand goes on the left side?

Of course not - the only argument ever presented for Yoda Conditions is
protection against the possibility of mistyping = instead of ==. Such
protection is only needed when one of the operands is an lvalue; such
protection is only possible if the other is not an lvalue. It is neither
needed nor possible in this case.

> What if it's unknown which of the operands is constant?
> Consider the expression (pNeedle == pHaystack).
> If we assume that pNeedle is variable and pHaystack is
> constant, this expression is, supposedly, perfectly readable.
> But if it turns out that pNeedle is constant and pHaystack is
> variable, the same expression suddenly becomes Yoda? So Yodaness

> is not a syntactic property of the expression itself, ...

The relevant property is lvalueness.

> ... but also

> depends on additional knowledge that may or may not be obvious from

> looking at the expression and its context, ...

If you're writing code so obfuscated that it leaves you uncertain which
expressions are lvalues, you've got bigger problems than can be dealt
with by using Yoda conditions.

> ... and that even may change
> at runtime?

Nothing that occurs at runtime can change whether or not a given
expression is an lvalue.

> I think the main purpose of the == operator is to test whether
> two values are equal. For that purpose it does not really matter
> whether operands are constant or not: it's equality we're
> interested in, not constness. Taking constness into consideration
> just complicates things unnecessarily; so let that not influence
> the way equalities are notated.
>
> By the way, does Yodaness apply to other commutative operators as
> well? Which one of (2 * f(x)) and (f(x) * 2) is Yoda, and why?

Do you know of any other commutative operators for which a common typo
converts them into a different operator for which one order is a
constraint violation, and the other is perfectly legal code that does
something quite different from what it would have done without the typo?
The other operator is, of course, necessarily not commutative.

There might be some other such pair of operators - I don't claim any
certainty about that. However, but I think the ==/= typo is
overwhelmingly the most common such pair.

[Malcolm McLean]

The +/= pair. They're generally on the same key, so

drawpixel(x + 2, y = 2);

is a common typo, and it's irritating that it compiles. However
unlike == / =, the difference applies to every language. Even
if you know C very well, often you don't see == / = mistakes,
whilst +/= leaps out.

[Keith Thompson]

Ike Naar <i...@iceland.freeshell.org> writes:
> On 2013-08-26, Keith Thompson <ks...@mib.org> wrote:
>> Ike Naar <i...@iceland.freeshell.org> writes:
>>> On 2013-08-25, Keith Thompson <ks...@mib.org> wrote:
>>>> James Kuyper <james...@verizon.net> writes:
>>>>> On 08/24/2013 03:05 PM, Sharwan Joram wrote:
>>>> [...]
>>>>>> if ( NULL == parameters[parametercount]){
>>>>
>>>> This is what's known as a "Yoda conndition"
>>>><http://en.wikipedia.org/wiki/Yoda_Conditions>. I know that a lot of
>>>> programmers like them, and for somewhat valid reasons, but personally I
>>>> find them jarring and unnecessary. Personally, I'd write that as:
>>>>
>>>> if (parameters[parametercount] == NULL) {
>>>
>>> Does it matter? The == operator is symmetric, (X==Y) == (Y==X).
>>>
>>> If (X==Y) is jarring and unnecessary, then, for symmetry reasons
>>> (Y==X) is unnecessary and jarring.
>> [...]
>>
>> Yes, it matters *to me* (and to plenty of other people). "==" is
>> commutative as far as the language is concerned, but code should be
>> written both for the compiler and for the human reader.
>>
>> X==Y is a poor example for this, since X==Y and Y==X are pretty much
>> equally readable.
>
> X and Y were meant to be placeholders for arbitrary subexpressions.

Sure, but my argument (whether you agree with it or not) doesn't apply
to arbitrary subexpressions.

> I prefer to treat (X==Y) as a mathematical formula (stating that
> subjects X and Y have equal values), rather than shoehorn it into
> an English sentence that has X, but not Y, as its subject.
>
> How about Yodaness when both operands are non-constant, as in
> (sin(alpha) == cos(beta)) ?.
> Would it now matter which operand goes on the left side?

The names "alpha" and "beta" might suggest that "alpha" goes first, but
in general it probably doesn't matter -- though it might depend on the
circumstances.

> What if it's unknown which of the operands is constant?
> Consider the expression (pNeedle == pHaystack).
> If we assume that pNeedle is variable and pHaystack is
> constant, this expression is, supposedly, perfectly readable.
> But if it turns out that pNeedle is constant and pHaystack is
> variable, the same expression suddenly becomes Yoda? So Yodaness
> is not a syntactic property of the expression itself, but also
> depends on additional knowledge that may or may not be obvious from
> looking at the expression and its context, and that even may change
> at runtime?

Strictly speaking, constness *is* a syntactic, or at least compile-time,
property of an expression, though it's going to depend on contact
outside the expression.

I'm not sure what to make of (pNeedle == pHaystack); normally one search
for a needle *in* a haystack.

Certainly you can construct a continuum of cases, with comparing a
computed value to a literal constant on one end, and comparing the
results of two clearly symmetric expressions on the other. I admit this
is rather vague, and I don't have a well-defined rule for when the order
is important to me.

> I think the main purpose of the == operator is to test whether
> two values are equal. For that purpose it does not really matter
> whether operands are constant or not: it's equality we're
> interested in, not constness. Taking constness into consideration
> just complicates things unnecessarily; so let that not influence
> the way equalities are notated.

But *why* do you want to know whether they're equal? What if, in the
problem domain, what you're really doing is asking a question about the
value of some expression (is it equal to 42?), and not asking a question
about 42 (about which you already have complete knowledge)? Putting the
constant on the right can provide a useful clue to the reader.

> By the way, does Yodaness apply to other commutative operators as
> well? Which one of (2 * f(x)) and (f(x) * 2) is Yoda, and why?

It matters far less for multiplication. I'm not sure I can clearly
articulate why.

>> The problem (for me) is when programmers write things
>> like:
>> if (0 == strcmp(s1, s2))
>> rather than
>> if (strcmp(s1, s2) == 0)
>>
>> When comparing a computed value to a constant, I'm asking a question
>> about the computed value: (Is it equal to this constant?) You can
>> ask a question about a constant: (Is it equal to this computed
>> value?), but I personally find that to be an awkward way to ask
>> the same question.
>
> It might help to re-phrase the question to one that is more symmetric
> in its operands, e.g. "are ... and ... equal?".

Logically true, but that doesn't make the Yoda form any clearer to me.
I know "==" is commutative, but (0 == strcmp(s1, s2)) is still clumsy
*to me*.

[snip]

>> I know why Yoda conditions are used, and it's a valid reason.
>> But would you even consider writing
>>
>> if (0 == strcmp(s1, s2))
>>
>> if it weren't for the "==" vs. "=" issue?
>
> Actually the Yoda form can be slightly more readable when the
> function call has a long argument list, as in
>
> if (3 == scanf("%g %g %g", &latitude, &longitude, &elevation))
>
> one can immediately see that the returnvalue of scanf is compared to 3,
> because the 3 is close to the scanf, whereas in
>
> if (scanf("%g %g %g", &latitude, &longitude, &elevation) == 3)
>
> the 3 and the scanf are far apart.

Ok, that's another valid reason for using Yoda conditions. I'd still put
the 3 on the right side myself; I might split it across two lines if I
thought the distance was a problem, or I might store the result in a
temporary.

But setting that aside for a moment (and avoiding the "!strcmp()"
variant):

It would never occur to me to write:
if (42 == x)
rather than
if (x == 42)
if it weren't for the possibility of confusing "==" and "=".
Would you write (42 == x) yourself? Do you really find it *just*
as natural as (x == 42)?

[Keith Thompson]

glen herrmannsfeldt <g...@ugcs.caltech.edu> writes:
> Keith Thompson <ks...@mib.org> wrote:
> (snip)
>>>> I know why Yoda conditions are used, and it's a valid reason.
>>>> But would you even consider writing
>
>>>> if (0 == strcmp(s1, s2))
>
>>>> if it weren't for the "==" vs. "=" issue?
>
> (snip, then I wrote)
>>> No, I would write
>
>>> if( ! strcmp(s1, s2))
>
>> Again, this is a matter of personal taste.
>
>> I know what !strcmp(s1, s2) means, but I dislike it.
>
> I suppose I don't especially like it, but I usually do it anyway.
>
>> Many functions return a logically Boolean result, even if the result
>> isn't of type bool or _Bool. By "logically Boolean", I mean that
>> the result answers a yes/no question without adding more information.
>
> Yes, but == and != don't help all that much. The relation is still
> backwards, but you get used to it. About as easy to get used to
> with ! as with !=.

It helps me.

if (strcmp(s1, s2) < 0) /* s1 is less than s2 */
if (strcmp(s1, s2) == 0) /* s1 is equal to s2 */
if (strcmp(s1, s2) > 0) /* s1 is greater than s2 */
if (strcmp(s1, s2) != 0) /* s1 is not equal to s2 */
if (strcmp(s1, s2) <= 0) /* s1 is less than or equal to s2 */
...

The result returned my strcmp is not (in my head, at least)
a Boolean; it's a tri-state comparison (with all negative values
logically equivalent to each other, and all positive values logically
equivalent to each other). Which is why the only thing I'm entirely
comfortable doing with strcmp() is explicitly comparing its result
to 0.

>> strcmp is not one of those functions, so I dislike using its result
>> either as a condition or as the operand of "!". For much the same
>> reason, I dislike writing "if (ptr)" and strongly prefer
>> "if (ptr != NULL)".
>
> In this case, I think of the pointer as pointing to something, or not,
> and so, to me, it makes some sense as a boolean.

And the language definition agrees with you -- but it's not *just* a
Boolean. A pure Boolean value does not have multiple distinct true
values; it's true or false, nothing else.

I'm sure part of it is that I learned Pascal before I learned C.
But when I see "if (ptr)", I have to mentally translate it to

"if (ptr != NULL)".

[...]

[Ike Naar]

Yes. Equality is a boolean function operating on two operands,
returning "the operands are equal". It does not matter which
one of the operands is on the left side of the operator.

[glen herrmannsfeldt]

Malcolm McLean <malcolm...@btinternet.com> wrote:

(snip)

>> The other operator is, of course, necessarily
>> not commutative.

> The +/= pair. They're generally on the same key, so

> drawpixel(x + 2, y = 2);

> is a common typo, and it's irritating that it compiles. However
> unlike == / =, the difference applies to every language. Even
> if you know C very well, often you don't see == / = mistakes,
> whilst +/= leaps out.

Not every language. Many don't allow assignment in the middle
of expressions. In Fortran, assignment is a statement, not an
operator.

In PL/I, assignment is also a statement, and = is the relational
operator.

x,y=1; /* multiple assignment */
x=y=1; /* relational operator */

-- glen

[Ian Collins]

James Kuyper wrote:
> On 08/26/2013 02:40 AM, Ike Naar wrote:

>> On 2013-08-25, Keith Thompson <ks...@mib.org> wrote:
>>> James Kuyper <james...@verizon.net> writes:
>>>> On 08/24/2013 03:05 PM, Sharwan Joram wrote:
>>> [...]
>>>>> if ( NULL == parameters[parametercount]){
>>>
>>> This is what's known as a "Yoda conndition"
>>> <http://en.wikipedia.org/wiki/Yoda_Conditions>. I know that a lot of
>>> programmers like them, and for somewhat valid reasons, but personally I
>>> find them jarring and unnecessary. Personally, I'd write that as:
>>>
>>> if (parameters[parametercount] == NULL) {
>>
>> Does it matter? The == operator is symmetric, (X==Y) == (Y==X).
>

> Yes, it matters - because it's intended to protect against the typo
> which replaces == with =, by ensuring that it would cause a constraint
> violation. The assignment operator is very definitely not symmetric.

The best way to protect against that particular typo is decent unit tests.

--
Ian Collins

[Ike Naar]

Do not overestimate the value of unit tests.
Let's take, for example, a piece of code that multiplies two
32-bit numbers to obtain a 64-bit result.
A decent test (that covers all cases) would require 2^64 test cases
which is at least impractical.

[James Kuyper]

On 08/26/2013 04:54 PM, Ian Collins wrote:
> James Kuyper wrote:
...

>> Yes, it matters - because it's intended to protect against the typo
>> which replaces == with =, by ensuring that it would cause a constraint
>> violation. The assignment operator is very definitely not symmetric.
>
> The best way to protect against that particular typo is decent unit tests.

I greatly prefer catching problems during compilation rather than testing.

[Ian Collins]

What has that got to do with this particular typo?

--
Ian Collins

[Ian Collins]

James Kuyper wrote:
> On 08/26/2013 04:54 PM, Ian Collins wrote:
>> James Kuyper wrote:

> ....

>>> Yes, it matters - because it's intended to protect against the typo
>>> which replaces == with =, by ensuring that it would cause a constraint
>>> violation. The assignment operator is very definitely not symmetric.
>>
>> The best way to protect against that particular typo is decent unit tests.
>
> I greatly prefer catching problems during compilation rather than testing.

So do I. For my projects make == make test.

--
Ian Collins

[Malcolm McLean]

On Monday, August 26, 2013 9:52:24 PM UTC+1, glen herrmannsfeldt wrote:

> Malcolm McLean <malcolm...@btinternet.com> wrote:
>
> > is a common typo, and it's irritating that it compiles. However
> > unlike == / =, the difference applies to every language. Even
> > if you know C very well, often you don't see == / = mistakes,
> > whilst +/= leaps out.
>
> Not every language. Many don't allow assignment in the middle
> of expressions. In Fortran, assignment is a statement, not an
> operator.
>

Every language except brainf**k and ilk uses + for addition, and virtually every one uses =, which is never used for addition.
(OK, go on, post some C++ which overloads the assignment
operator).

So the +/= typo applies to every language. It will always generate
an expression which is wrong, in every language known to the
programmer. It leaps out.

OTOH if(Nstates = 52) is correct Fortran with the obvious meaning.
In C, the logic will break when Texas secedes, but it will
appear to work until then. So the error doesn't leap out.

[Keith Thompson]

Fascinating.

You are of course absolutely correct with respect to the semantics
of the "==" operator; nevertheless (as I've already said) I find
(42 == x) annoyingly awkward -- as do many other people. I take
your word for it that you don't, but I honestly find it surprising.

[Ike Naar]

Everything. If you know the typo in your progam, you
don't need tests to find it, you just fix the typo.
If you don't know there's a typo, the only way to be sure you
find one is to test all possible cases.

[Ian Collins]

But you will know as soon as you add the code and it fails its test.

--
Ian Collins

[Ike Naar]

On 2013-08-26, Keith Thompson <ks...@mib.org> wrote:
> Ike Naar <i...@iceland.freeshell.org> writes:
>> On 2013-08-26, Keith Thompson <ks...@mib.org> wrote:
>>> It would never occur to me to write:
>>> if (42 == x)
>>> rather than
>>> if (x == 42)
>>> if it weren't for the possibility of confusing "==" and "=".
>>> Would you write (42 == x) yourself? Do you really find it *just*
>>> as natural as (x == 42)?
>>
>> Yes. Equality is a boolean function operating on two operands,
>> returning "the operands are equal". It does not matter which
>> one of the operands is on the left side of the operator.
>
> Fascinating.
>
> You are of course absolutely correct with respect to the semantics
> of the "==" operator; nevertheless (as I've already said) I find
> (42 == x) annoyingly awkward -- as do many other people. I take
> your word for it that you don't, but I honestly find it surprising.

Do you find (42 + x) more or less awkward than (x + 42), and why?
If you don't care, then what's the fundamental difference between
the + operator and the == operator?

[Ike Naar]

That depends. If you're lucky, bad code will fail the tests. If
you're not lucky (i.e. if the test subset did not cover all possible
failures) bad code may pass the tests. As long as the tests don't
cover all possible failures, passing the tests does not guarantee
the code is okay.

[gdo...@gmail.com]

On Monday, August 26, 2013 7:58:47 AM UTC-4, Ben Bacarisse wrote:
> gdo...@gmail.com writes:

> <snip>
char **a;
int numberOfPointers = 5;


a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );
if ( a!= NULL )
printf( "the array, a, points to declare space\n\n" );

char *b[] = { "one", "two", "three", "four", "five" };

a = b;

> It's possible that you don't know what this assignment does. It only
> replaces one pointer with another. The pointer value that used to be in
> 'a' is lost so you can no longer access the memory that was allocated.
> in particular, you can't ever free it now.
> <snip>

Ben, so a=b; , does not assign a to point to whatever b points to?
it replaces?
I thought it was having a point to the same memory location.
How would you do that? How would you have a point to b? what is the syntax?

thanks.

[glen herrmannsfeldt]

Ike Naar <i...@iceland.freeshell.org> wrote:

(snip, someone wrote)

>> You are of course absolutely correct with respect to the semantics
>> of the "==" operator; nevertheless (as I've already said) I find
>> (42 == x) annoyingly awkward -- as do many other people. I take
>> your word for it that you don't, but I honestly find it surprising.

> Do you find (42 + x) more or less awkward than (x + 42), and why?
> If you don't care, then what's the fundamental difference between
> the + operator and the == operator?

Algebra commonly puts the coefficient before the variable,
so 2x and not x2. Polynomials are commonly written in order
of decreasing power of x, so x+42 instead if 42+x.

-- glen

[James Kuyper]

On 08/26/2013 05:54 PM, gdo...@gmail.com wrote:
> On Monday, August 26, 2013 7:58:47 AM UTC-4, Ben Bacarisse wrote:
>> gdo...@gmail.com writes:
>
>> <snip>
> char **a;
> int numberOfPointers = 5;
>
>
> a = (char **) malloc ( numberOfPointers * ( sizeof(char * ) ) );
> if ( a!= NULL )
> printf( "the array, a, points to declare space\n\n" );
>
> char *b[] = { "one", "two", "three", "four", "five" };
>
> a = b;
>
>> It's possible that you don't know what this assignment does. It only
>> replaces one pointer with another. The pointer value that used to be in
>> 'a' is lost so you can no longer access the memory that was allocated.
>> in particular, you can't ever free it now.
>> <snip>
>
> Ben, so a=b; , does not assign a to point to whatever b points to?

C has special many special rules for arrays, like b. One of those rules
says that, in most context, an lvalue of array type is automatically
converted into a pointer to the first element of that array. This is one
of those contexts. There are three exceptions to that rule:

char *(*c)[5] = &b;

'b' does not get converted into a pointer to it's first element; instead
it remains an array, so &b has the type char*(*)[5].

The second exception is
size_t array_size = sizeof b;
which gives the size of b, and not the size of a pointer to the first
element of b.

The third exception is:
char array[] = "string literal";
where the string literal is an lvalue of array type, but instead of
being automatically converted into a pointer to char (which would be a
constraint violation in this context), it gets used to initialize the
array, exactly as if I had written:
char array[] = {'s', 't', 'r', 'i', 'n', 'g', ' ',
'l', 'i', 't', 'e', 'r', 'a', 'l', '\0'};

In all other contexts, an lvalue of array type gets automatically
converted into a pointer to the first element of the array. This rule
often leads people into confusing pointers and arrays, but they are
quite different (though closely related) things.

As a result of that conversion, the line "a=b;" causes a to contain a
pointer to the first element of b.

> it replaces?

Yes, just like numberOfPointers = 6 would cause the value of 5, which is
currently stored in numberOfPointers, to be replaced with the value of
6. That isn't much of a problem, unless you have some need to know what
the old value was - then it becomes a BIG problem.

In this case, before the line "a=b;", the variable 'a' contained a
pointer to memory that had been allocated by a call to malloc(). After
the assignment, 'a' contained a pointer to the first element of b. There
is no other copy of the pointer that used to be stored in 'a'. That
means you no longer have any way to access the memory that the pointer
pointed at. This also means you have no way of freeing that memory.

> I thought it was having a point to the same memory location.
> How would you do that? How would you have a point to b? what is the syntax?

If you want a to point at the first element of 'b', then there's
absolutely nothing wrong with using "a=b;". The problem is not about
having 'a' point at 'b'. However, 'a' can only point at one thing, it
can either point at the memory you allocated with a call to malloc(), or
it can point at 'b'; it can't point at both. If you want to point at two
different things, then you need two different pointers.

Of course, in this context, it would be a good question to ask why you
want to point at two different things. It would be an even better
question to ask why you have two different things to point at - your
code makes no use of the memory allocated by that call to malloc(), so
your program has no obvious reason for bothering to allocate that memory
in the first place.

What did you think you were allocating that memory for? Whatever it was,
your code isn't actually doing it.

[Keith Thompson]

Ike Naar <i...@iceland.freeshell.org> writes:
> On 2013-08-26, Keith Thompson <ks...@mib.org> wrote:
>> Ike Naar <i...@iceland.freeshell.org> writes:
>>> On 2013-08-26, Keith Thompson <ks...@mib.org> wrote:
>>>> It would never occur to me to write:
>>>> if (42 == x)
>>>> rather than
>>>> if (x == 42)
>>>> if it weren't for the possibility of confusing "==" and "=".
>>>> Would you write (42 == x) yourself? Do you really find it *just*
>>>> as natural as (x == 42)?
>>>
>>> Yes. Equality is a boolean function operating on two operands,
>>> returning "the operands are equal". It does not matter which
>>> one of the operands is on the left side of the operator.
>>
>> Fascinating.
>>
>> You are of course absolutely correct with respect to the semantics
>> of the "==" operator; nevertheless (as I've already said) I find
>> (42 == x) annoyingly awkward -- as do many other people. I take
>> your word for it that you don't, but I honestly find it surprising.
>
> Do you find (42 + x) more or less awkward than (x + 42), and why?

No. Well, now that I think about it, I think I prefer (x + 42), for
reasons I can't necessarily articulate.

> If you don't care, then what's the fundamental difference between
> the + operator and the == operator?

The difference is that "==" is much more natural with the constant on
the right, and with "+" the difference is less striking.

I know that doesn't answer your question.

"Fred is 42 years old" vs. "42 years old is Fred" might shed *some*
light on it, but it's not a great example ("is" in that context does not
denote equality).

[Keith Thompson]

`a = b` copies the value of b into a (and is legal only if a
and b are of the same type, or if b is implicitly convertible to
a's type.). That's true whether a and b are integers, structures,
pointers, or anything else that can be assigned. So if a and b are
pointers, then `a = b` copies the value of b (which is an address,
possibly a null pointer) into the object a. After the assignment,
`a == b`, which means either that a and b are both null pointers
or that they both point to the same thing.

To cause a to point to b:

a = &b;

This is legal only if a is a pointer to some type FOO, and b is of type
FOO.

To copy the value of the object b points to into the object that a
points to:

*a = *b;

After this, `a == b` is not necessarily true, but `*a == *b` is true.