| Message ID | 20240206061721.27297-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Mini-OS: fix 9pfs frontend error path | expand |
Juergen Gross, le mar. 06 févr. 2024 07:17:21 +0100, a ecrit: > The early error exit in p9_stat() returns without zeroing the p9_stat > buffer, resulting in free() being called with an uninitialized pointer. > > Fix that by doing the zeroing first. This is not coherent with the usual conventions: when a function fails, it is supposed not to have done anything, and thus the caller shouldn't have to clean anything. I.e. i'd rather see the free_stat() call be put after the check for an error returned by p9_stat. > Reported-by: Julien Grall <julien@xen.org> > Fixes: 2d1dfccd3aa3 ("Mini-OS: add read and write support to 9pfsfront") > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > 9pfront.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/9pfront.c b/9pfront.c > index 315089bc..33eaadce 100644 > --- a/9pfront.c > +++ b/9pfront.c > @@ -716,10 +716,11 @@ static int p9_stat(struct dev_9pfs *dev, uint32_t fid, struct p9_stat *stat) > uint16_t total; > int ret; > > + memset(stat, 0, sizeof(*stat)); > + > if ( !req ) > return EAGAIN; > > - memset(stat, 0, sizeof(*stat)); > req->cmd = P9_CMD_STAT; > send_9p(dev, req, "U", fid); > rcv_9p(dev, req, "uuuUQUUULSSSSSUUU", &total, &stat->size, &stat->type, > -- > 2.35.3
On 06.02.24 16:26, Samuel Thibault wrote: > Juergen Gross, le mar. 06 févr. 2024 07:17:21 +0100, a ecrit: >> The early error exit in p9_stat() returns without zeroing the p9_stat >> buffer, resulting in free() being called with an uninitialized pointer. >> >> Fix that by doing the zeroing first. > > This is not coherent with the usual conventions: when a function fails, > it is supposed not to have done anything, and thus the caller shouldn't > have to clean anything. > > I.e. i'd rather see the free_stat() call be put after the check for > an error returned by p9_stat. I can do that, but this would require two calls of free_stat() (one in p9_stat() in an error case reported via req->result, and one in the caller of p9_stat() in case of no error). Juergen
Jürgen Groß, le mar. 06 févr. 2024 16:37:17 +0100, a ecrit: > On 06.02.24 16:26, Samuel Thibault wrote: > > Juergen Gross, le mar. 06 févr. 2024 07:17:21 +0100, a ecrit: > > > The early error exit in p9_stat() returns without zeroing the p9_stat > > > buffer, resulting in free() being called with an uninitialized pointer. > > > > > > Fix that by doing the zeroing first. > > > > This is not coherent with the usual conventions: when a function fails, > > it is supposed not to have done anything, and thus the caller shouldn't > > have to clean anything. > > > > I.e. i'd rather see the free_stat() call be put after the check for > > an error returned by p9_stat. > > I can do that, but this would require two calls of free_stat() (one in > p9_stat() in an error case reported via req->result, and one in the > caller of p9_stat() in case of no error). Indeed, but that still looks more coherent with usual conventions. Samuel
diff --git a/9pfront.c b/9pfront.c index 315089bc..33eaadce 100644 --- a/9pfront.c +++ b/9pfront.c @@ -716,10 +716,11 @@ static int p9_stat(struct dev_9pfs *dev, uint32_t fid, struct p9_stat *stat) uint16_t total; int ret; + memset(stat, 0, sizeof(*stat)); + if ( !req ) return EAGAIN; - memset(stat, 0, sizeof(*stat)); req->cmd = P9_CMD_STAT; send_9p(dev, req, "U", fid); rcv_9p(dev, req, "uuuUQUUULSSSSSUUU", &total, &stat->size, &stat->type,
The early error exit in p9_stat() returns without zeroing the p9_stat buffer, resulting in free() being called with an uninitialized pointer. Fix that by doing the zeroing first. Reported-by: Julien Grall <julien@xen.org> Fixes: 2d1dfccd3aa3 ("Mini-OS: add read and write support to 9pfsfront") Signed-off-by: Juergen Gross <jgross@suse.com> --- 9pfront.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)