[BusyBox] xfuncts.c xfree() ?

[Neal H Walfield]

> If there is a variable that may or may not be assigned data (depending
> on some condition) then its easier to free it unconditionally rather
> than testing it first.

This is completely wrong.
>
> e.g. in unarchival/libunarchive/unarchive.c at the bottom of the file it
> does
>
> 	free(file_entry->name);
> 	free(file_entry->link_name);
> 	free(file_entry);

Good example.

Consider the following:

The first thing that we do in extract_archive is check if prefix is
NULL, if it is not, we set full_name to a fresh allocated buffer,
otherwise, we point it to file_entry->name.

        char *extract_archive(FILE *src_stream, FILE *out_stream,
                              const file_header_t *file_entry,
                              const int function, const char *prefix)
        {
        	FILE *dst_stream = NULL;
                char *full_name = NULL;
        	char *buffer = NULL;
        	struct utimbuf t;

        	/* prefix doesnt have to be a proper path it may prepend 
        	 * the filename as well */
        	if (prefix != NULL) {
        		/* strip leading '/' in filename to extract as prefix may not be dir */
                	/* Cant use concat_path_file here as prefix might not be a directory */
                        char *path = file_entry->name;
        		if (strncmp("./", path, 2) == 0) {
        			path += 2;
        			if (strlen(path) == 0) {
        				return(NULL);
                		}
        		}
        		full_name = xmalloc(strlen(prefix) + strlen(path) + 1);
        		strcpy(full_name, prefix);
        		strcat(full_name, path);
        	} else {
        		full_name = file_entry->name;
        	}

At the end of the function, we unconditionally free full_name:

        	free(full_name);

        	return(NULL); /* Maybe we should say if failed */
        }

Because we have not kept track of who owns full_name, we may have now
freed file_entry->name.  Is this an the correct thing to do?  Let's
take a look at a caller of extract_archive, e.g. the same file,
unarchive, line 264:

		if (extract_flag == TRUE) {
			buffer = extract_archive(src_stream, out_stream, file_entry, extract_function, prefix);
		} else {
			/* seek past the data entry */
			seek_sub_file(src_stream, file_entry->size);
		}
		free(file_entry->name); /* may be null, but doesn't matter */
		free(file_entry->link_name);
		free(file_entry);


We call extract_archive and then free file_entry->name, however, this
may have already been freed by extract_archive!

Correct me if my logic is wrong, however, I am not that familiar with
the code.

Not to mention that in the case of returning early, e.g. lines 109,
126, 152, 173 and 184, full_name is _not_ free'd!

> file_entry->link_name as you can guess will only ever have data if the
> file is a sysmlink.
>
> If its wrapped in an if to prevent it from being free'ed if its NULL
> i.e.
>
> if (file_entry->link_name) {
> 	free(file_entry->link_name);
> }
>
> then it increases the size of the binary for no functional
> advantage.

Well, bothering to set the variable to NULL initially wastes space
too.