[prog] [C] strings bug

[Laurel Fan]

See explanation inline with the quoted code:

On Wed, Apr 09, 2003 at 10:55:02PM +0100, Conor Daly wrote:
> char *strset(char *dest, const char *src) {
> 	/* This is just a wrapper for strdup() which handles
> 	 * all the necessary stuff
> 	 * Since strdup() allocates a new string, we need to
> 	 * set the target to become the returned pointer
> 	 *
> 	 * Useage: dest = strset(dest, src);
> 	 *
> 	 * Unfortunately, we need the address of the old string so we 
> 	 * can free that, otherwise we get a memory leak.
> 	 *
> 	 */
> 
> 
> 	/* create a tmp string to hold 'dest' while we free that */
> 	char *tmp;

The problem is with the inital value of tmp.  When you declare a local
variable, its initial value is undefined.  See below...

> 	return(strdup(src));/* This is a memory leak */
> 	/* comment out the line above to see the bug in action */
> 
> 	printf("in function strset()\n");
> 	if(dest)
> 	{
> 
> 	/* take a copy of dest */
> 	if( (tmp = strdup(dest)) == NULL)

tmp might be set here, but only if dest was not NULL.

> 		{
> 			return(dest); /* we failed to make a copy so bail
> 					out */
> 		}
> 	/* then free it */
> 	free(dest);
> 	}
> 	/* now use strdup() to replace the old string with the new */
> 	if( (dest = strdup(src)) == NULL) {
> 		  printf("Failed to set dest\n");
> 	/* the copy failed so we want to return the original string
> 	unchanged.
> 	 */
> 		return(tmp);
> 	} else {
> 		/* free the tmp string */
> 		free(tmp);

Here, it is possible that the value of tmp is not defined (which means
it could be anything), so freeing this value can have unpredictable
results.

> 		/* and return the new pointer to dest */
> 		return(dest);
> 	}
> }
> /* *************end strset()**************************/

That said, the way I would do it avoids copying dest altogether:

char *strset(char *string_to_replace, onst char *src) {
    /*
        Return a newly allocated copy of src.
        If successful, string_to_replace will be freed.
        If unsuccessful, no new memory will be allocated, and
	string_to_replace will be returned unchanged.
    */

    char *new_copy_of_src = strdup(src);

    if (new_copy_of_src == NULL) {
        /* printf("Copying src string failed\n"); */
        return(string_to_replace);
    } else {
	/* printf("Copying src string succeeded\n") */
        free(string_to_replace);
        return(new_copy_of_src);
    }
}


-- 
laurel at sdf.lonestar.org
http://dreadnought.gorgorg.org