Patchwork [2/8] fedfsd: don't return freed memory through **pathname parameter, ...

login
register
mail settings
Submitter Jim Meyering
Date Nov. 30, 2011, 8:51 p.m.
Message ID <1322686298-14634-3-git-send-email-jim@meyering.net>
Download mbox | patch
Permalink /patch/128577/
State Superseded
Headers show

Comments

Jim Meyering - Nov. 30, 2011, 8:51 p.m.
From: Jim Meyering <meyering@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(-)
Chuck Lever - Nov. 30, 2011, 9:13 p.m.
On Nov 30, 2011, at 3:51 PM, Jim Meyering wrote:

> From: Jim Meyering <meyering@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

	if (retval != FEDFS_OK) {
		free(result);
		return retval;
	}
	*pathname = result;
	return FEDFS_OK;

> 		return retval;
> 	}
> 
> -- 
> 1.7.8.rc4
> 
> 
> _______________________________________________
> fedfs-utils-devel mailing list
> fedfs-utils-devel@oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/fedfs-utils-devel
Jim Meyering - Nov. 30, 2011, 9:20 p.m.
Chuck Lever wrote:

> On Nov 30, 2011, at 3:51 PM, Jim Meyering wrote:
>
>> From: Jim Meyering <meyering@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;
	}
Chuck Lever - Nov. 30, 2011, 9:31 p.m.
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@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@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.

Patch

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;
 		return retval;
 	}