[fedfs-utils] [PATCH 2/8] fedfsd: don't return freed memory through **pathname parameter, ...

[Chuck Lever]

On Nov 30, 2011, at 4:20 PM, Jim Meyering wrote:

> Chuck Lever wrote:
> 
>> On Nov 30, 2011, at 3:51 PM, Jim Meyering wrote:
>> 
>>> From: Jim Meyering <meyering at redhat.com>
>>> 
>>> even though upon error it will not be used.
>>> * src/fedfsd/svc.c (fedfsd_pathwalk): On an error path, don't
>>> set *pathname at all, and certainly not to a just-freed pointer.
>>> 
>>> mount avoid one-byte heap-write overrun
>>> ---
>>> src/fedfsd/svc.c |    1 -
>>> 1 files changed, 0 insertions(+), 1 deletions(-)
>>> 
>>> diff --git a/src/fedfsd/svc.c b/src/fedfsd/svc.c
>>> index 132be70..3509082 100644
>>> --- a/src/fedfsd/svc.c
>>> +++ b/src/fedfsd/svc.c
>>> @@ -318,7 +318,6 @@ fedfsd_pathwalk(const FedFsPathName fpath, char **pathname)
>>> 		retval = fedfsd_pathwalk_check_term(result);
>>> 		if (retval != FEDFS_OK)
>>> 			free(result);
>>> -		*pathname = result;
>> 
>> if retval is FEDFS_OK, we want to preserve the result.
>> So the existing code is incorrect, but this fix is also wrong.  How about
> 
> Oh.  How embarrassing.
> 
>> 	if (retval != FEDFS_OK) {
>> 		free(result);
>> 		return retval;
>> 	}
>> 	*pathname = result;
>> 	return FEDFS_OK;
>> 
>>> 		return retval;
>>> 	}
> 
> How about this instead?
> This way there's a single return point and no duplicated FEDFS_OK:
> 
> 	if (fpath.FedFsPathName_len == 0) {
> 		xlog(D_CALL, "%s: Zero-component pathname", __func__);
> 		strcat(result, "/");
> 		retval = fedfsd_pathwalk_check_term(result);
> 		if (retval == FEDFS_OK)
> 			*pathname = result;
> 		else
> 			free(result);
> 		return retval;
> 	}

The pattern is generally:

	if (error)
		do exception handling
	do the non-error case

I call this bfields-normal form, after bfields at redhat.com, as he finds this logic easier to read than other ways to express it.  I don't typically use the "else" form elsewhere in this code.

The "return FEDFS_OK" nails the return code instead of letting it leak through.  That tends to be more immune to changes in the code around it.  The small code duplication doesn't bug me.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com