| Message ID | 1322686298-14634-3-git-send-email-jim@meyering.net |
|---|---|
| State | Superseded |
| Headers | show |
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
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; }
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.
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; }
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(-)