[prog] [C] strings bug

[Meredydd Luff]

On Wednesday 09 April 2003 22:55, Conor Daly wrote:
> /* This is main.c */
> #include<stdio.h>
> #include<string.h>
> #include<stdarg.h>
>
> int main(void);
> int printout(void);
> char *strdup2(const char *, const char *);
> char *strset(char *, const char *);
> char *stradd(char *, const char *);
>
> 	char *string1;
> 	char *string2;
> 	char *string3;
Should these be local to main()? Oh, I see, you're using globals to pass the 
values to printout(). That's ugly and shouldn't be used outside this little 
testing hack, but then I'm pretty sure you already knew that :)

>
> int main(void)
> {
>
> 	printout();
When this function is called, the values of string1, string2, and string3 are 
undefined - they could technically be anything. Looking at your output, 
you're in luck. They seem to have been set to NULL, but don't rely on this 
happening. These three pointers could be pointing anywhere right now - NULL, 
somewhere in the middle of your program's memory space, somewhere in some 
other program's memory space, somewhere in the middle of nowhere... Of 
course, accesses to either of those last two will be caught and result in a 
SIGSEGV. It's clobbering your own data you have to be wary of.

> 	string1 = strset(string1, "Test");/**/
What's the desired result of this? Looking at your implementation of strset() 
a little further down, it looks like what you're doing could just be 
accomplished by:

char * strset(char * dest, char * src)
{
  if(dest!=NULL) { free(dest); }

  return strdup(src);
}

Correct me if I'm wrong, but the error condition in which malloc() (which is 
called by strdup()) returns NULL only occurs when the machine has run out of 
both memory and swap space, by which point you're so hosed that one little 
program segfaulting isn't going to make that much of a difference (by that 
point you can't even halt the machine, because there's no memory to spawn the 
"shutdown" process). In any case, the program should "fail fast" on that type 
of error - an immediate bail-out is much easier to debug than subtle bugs 
caused by the old contents of a string sticking around past an assignment, 
and even if you want to bail out gracefully rather than with a SIGSEGV, 
checking whether a pointer is NULL is much faster and easier than a strcmp() 
to check whether it has changed.


> 	string3 = strset(string3, "Addition");
> 	printout();
You still haven't set string2 yet - again, this time it was conventiently 
NULL, but don't bank on it.

Despite all of the above, the lucky coincidence of them all being initially 
NULL has meant that both assignments so far have worked out fine.

> 	string2 = strset(string2, "Test");/**/
> 	string1 = strset(string1, "Testing");/**/
> 	printout();
>From the results of printout() here, it looks like something awful has 
happened to string3 over the course of those two strset() calls. This is a 
bug in strset(), but I'll get onto that later. Using the function I knocked 
up above, it works fine.

> 	string1 = stradd(string1, ", Testing...");/**/
> 	string3 = strdup2(string3, "Addition");
> 	printout();
Again, with those minor modifications to strset(), this works fine too.

> 	printf("%s, %s\n",  strset(string3, "Direct print"),  strset(string3, " on
> screen")); }
Now here we get problems. My implementation of strset() segfaults here, and 
with good reason. The point of strset(), is it not, is to free "dest" if 
necessary, and then strdup() src and return the new pointer. The way that it 
tells whether freeing src is necessary is whether or not it is NULL. This 
works fine, as long as you always assign the return value of strset() to 
whatever you passed as its first argument ("dest"). Here, however, you don't 
do that, so you free() the are of memory pointed to by string3 twice. 
Normally (always?), this results in a segfault.

Now, we could do this:
  printf("%s, %s\n",  (string3=strset(string3, "Direct print")),
	(string3=strset(string3, "on screen")) );
Unfortunately, this won't work either. The reason is that when the C compiler 
compiles things, it unrolls that line into something like this:

char * p1, * p2, * p3;

p1="%s, %s\n";
p2=(string3=strset(string3, "Direct print"));
p3=(string3=strset(string3, "on screen"));

printf(p1, p2, p3);

Do you see the problem? The bit of memory pointed to by p2 (and, at the time 
of assignment, string3 too) is free()ed by the line that assigns p3. By the 
time printf() gets called, it's being passed a pointer to unallocated memory.

OK, just a quick comment on the problem with this strset():

> /* *************strset()**************************
>  * Uses strdup() to set an existing string
>  */
> char *strset(char *dest, const char *src) {
>
> 	/* create a tmp string to hold 'dest' while we free that */
> 	char *tmp;
Again, tmp isn't assigned, and could be anything.

>
> 	printf("in function strset()\n");
> 	if(dest)
> 	{
>
> 	/* take a copy of dest */
> 	if( (tmp = strdup(dest)) == NULL)
> 		{
> 			return(dest); /* we failed to make a copy so bail
> 					out */
> 		}
> 	/* then free it */
> 	free(dest);
> 	}
OK, that's all well and good, but what happens if dest==NULL? tmp doesn't get 
touched - but below, it gets free()d, whatever it points to! In the test run 
you gave, I suspect that by pure coincidence, it ended up pointing to the bit 
of memory that string3 was also pointing at, so when that free() call 
happened, it freed the third string, which promptly got clobbered by 
something else.

Right, I hope that helps. Do yell at me if it didn't :^)

Meredydd
-- 

Everybuddy project maintainer
http://www.everybuddy.com/

MSN:     blip109 at hotmail.com
AIM:     blip109
Yahoo:   modula7