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