Messages in this thread | | | Date | Thu, 26 Oct 2023 20:53:55 +0900 | From | asmadeus@codewrec ... | Subject | Re: [PATCH] net: 9p: fix possible memory leak in p9_check_errors() |
| |
Hangyu Hua wrote on Thu, Oct 26, 2023 at 05:23:51PM +0800: > When p9pdu_readf is called with "s?d" attribute, it allocates a pointer > that will store a string. But when p9pdu_readf() fails while handling "d" > then this pointer will not be freed in p9_check_errors.
Right, that sounds correct to me.
Out of curiosity how did you notice this? The leak shouldn't happen with any valid server.
This cannot break anything so I'll push this to -next tomorrow and submit to Linus next week
> Fixes: ca41bb3e21d7 ("[net/9p] Handle Zero Copy TREAD/RERROR case in !dotl case.")
This commit moves this code a bit, but the p9pdu_readf call predates it -- in this case the Fixes tag is probably not useful; this affects all maintained kernels.
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > --- > net/9p/client.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/9p/client.c b/net/9p/client.c > index 86bbc7147fc1..6c7cd765b714 100644 > --- a/net/9p/client.c > +++ b/net/9p/client.c > @@ -540,12 +540,15 @@ static int p9_check_errors(struct p9_client *c, struct p9_req_t *req) > return 0; > > if (!p9_is_proto_dotl(c)) { > - char *ename; > + char *ename = NULL; > > err = p9pdu_readf(&req->rc, c->proto_version, "s?d", > &ename, &ecode); > - if (err) > + if (err) { > + if (ename != NULL) > + kfree(ename);
Don't check for NULL before kfree - kfree does it. If that's the only remark you get I can fix it when applying the commit on my side.
> goto out_err; > + } > > if (p9_is_proto_dotu(c) && ecode < 512) > err = -ecode;
-- Dominique Martinet | Asmadeus
| |