misc/114110: Disk_Names() returns a vector that is unsafe to free

[David Sanderson]

>Number:         114110
>Category:       misc
>Synopsis:       Disk_Names() returns a vector that is unsafe to free
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Jun 28 21:50:01 GMT 2007
>Closed-Date:
>Last-Modified:
>Originator:     David Sanderson
>Release:        FreeBSD 6.2
>Organization:
Panasas, Inc.
>Environment:
FreeBSD perf-x4 6.2-RELEASE FreeBSD 6.2-RELEASE #0: Fri Jan 12 08:43:30 UTC 2007     root at portnoy.cse.buffalo.edu:/usr/obj/usr/src/sys/SMP  amd64

>Description:
The documentation for the Disk_Names() function in libdisk(3) states:

     Disk_Names() returns `char**' with all disk's names (wd0, wd1 ...).  You
     must free each pointer, as well as the array by hand.

However, looking at the code, in FreeBSD 6 this routine returns a vector that
can be freed, but only one of the pointers in that vector can be freed, because
they all point to space in a single malloced buffer, char *disklist.  Since the pointers in the vector are sorted by the strings they point to, there is no way for the caller to know which is safe to free, and it is certainly wrong to free them all.

It would be best for Disk_Names() to return a vector according to its docs.
>How-To-Repeat:

>Fix:
I'll contribute a sample new version that returns a vector that is safe to free according to the docs.

Patch attached with submission follows:

char **
Disk_Names()
{
	static char **disks;
	int error;
	size_t listsize;
	char *disklist;
        char *disklistp;
        size_t ndisks;
        size_t i;

	error = sysctlbyname("kern.disks", NULL, &listsize, NULL, 0);
	if (error) {
		warn("kern.disks sysctl not available");
		return NULL;
	}

	if (listsize == 0)
		return (NULL);

	disklist = (char *)malloc(listsize + 1);
	if (disklist == NULL) {
		return NULL;
	}
	memset(disklist, 0, listsize + 1);
	error = sysctlbyname("kern.disks", disklist, &listsize, NULL, 0);
	if (error || disklist[0] == 0) {
		free(disklist);
		return NULL;
	}
	for (disklistp = disklist, ndisks = 0; disklistp;) {
		char *disk = strsep(&disklistp, " ");
		if (disk) {
			/* We restore the space for a second pass through the names. */
			if (disklistp) {
				disklistp[-1] = ' ';
			}
			ndisks++;
		}
	}
	disks = malloc(sizeof *disks * (1 + ndisks));
	if (disks == NULL) {
		free(disklist);
		return NULL;
        }
	memset(disks,0,sizeof *disks * (1 + ndisks));
	for (disklistp = disklist, i = 0; i < ndisks; i++) {
		disks[i] = strdup(strsep(&disklistp, " "));
		if (disks[i] == NULL) {
			size_t j;
			for (j = 0; j < i; j++) {
				free(disks[j]);
			}
			free(disklist);
			free(disks);
			return NULL;
		}
	}
        free(disklist);
	qsort(disks, ndisks, sizeof(char*), qstrcmp);
	return disks;
}


>Release-Note:
>Audit-Trail:
>Unformatted: