[prog] [C] strings bug

[Conor Daly]

On Thu, Apr 10, 2003 at 09:50:51AM +0100 or thereabouts, Meredydd Luff wrote:
> On Wednesday 09 April 2003 22:55, Conor Daly wrote:
> >
> > 	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, 

Again, that's just for the test.  I'm pretty sure I don't print anything
undefined in the real code...
 
> > 	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.

True.  All true.  The idea was to have the string assignment happen without
having to do the checking in the main code.  Otherwise, I could do

free(dest);
dest = strdup("Test");
/* conduct checks here */

in my main code.  I'm not sure that I've done that though...
 
> 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.

Is it not the case that (as detailed below) the call unrolls into:

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

On the second call to strset, string3 should be NULL having been free()ed by
the first call and therefore shouldn't be free()ed again.
 
> 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.

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

Helps lots, thanks,

Conor
-- 
Conor Daly 
Met Eireann, Glasnevin Hill, Dublin 9, Ireland
Ph +353 1 8064276 Fax +353 1 8064247
------------------------------------
bofh.irmet.ie running RedHat Linux  9:12am  up 6 days, 23:03, 10 users,  load average: 0.00, 0.04, 0.01


**********************************************************************
This e-mail and any files transmitted with it are confidential 
and intended solely for the addressee. If you have received
this email in error please notify the sender.
This e-mail message has also been scanned for the
presence of computer viruses.
**********************************************************************