9pfront.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
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(-)
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
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
© 2016 - 2024 Red Hat, Inc.