There are valid points especially about possible usage in memprintf :-)
On 21 September 2017 at 16:28, Willy Tarreau <w...@1wt.eu> wrote:
> Hi David,
>
> On Thu, Sep 21, 2017 at 04:15:51PM +0100, David CARLIER wrote:
> > Hi all,
> >
> > At first my patch was supposed to be a bit more important but either some
> > changes are already taking care of or after second thoughts some were not
> > worthy enough.
>
> Thanks!
>
> > Hope it s good enough.
>
> Just a few comments (I always have comments ;-))
>
> > Kind regards.
>
> > From 4c694b4ff20a6ec88bc73c51a93c23de3f781c01 Mon Sep 17 00:00:00 2001
> > From: David Carlier <devne...@gmail.com>
> > Date: Thu, 21 Sep 2017 14:36:43 +0000
> > Subject: [PATCH] CLEANUP: log
>
> Please be a bit more indicative on the subject line so that when we work
> on backports to -stable and we use "git log --oneline old..new" we can at
> least guess whether or not this has to be considered for backporting. Also
> in my opinion it's a bug since it's a memory leak (though a minor one). In
> this case I would suggest such a subject line :
>
> BUG/MINOR: log: don't leak memory on error path in logformat parser
>
> > since we do not log the sample fetch when it is invalid, we can
> > free the log data.
>
> That's perfect for the description.
>
> > ---
> > src/log.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/src/log.c b/src/log.c
> > index 5c42f6b2a..0fb0f68e0 100644
> > --- a/src/log.c
> > +++ b/src/log.c
> > @@ -484,6 +484,7 @@ int add_sample_to_logformat_list(char *text, char
> *arg, int arg_len, struct prox
> > node->options |= LOG_OPT_RES_CAP; /* fetch method is
> response-compatible */
> >
> > if (!(expr->fetch->val & cap)) {
> > + free(node);
>
> You have some indent issues here, it's using spaces and doesn't appear
> correctly. I suspect you have configured your editor with a certain tab
> sizes making this one not visible on screen and only in the patch.
>
> > memprintf(err, "sample fetch <%s> may not be reliably used
> here because it needs '%s' which is not available here",
> > text, sample_src_names(expr->fetch->use));
> > return 0;
>
> Another thing is that in order to avoid use-after-free errors, we always
> null a pointer that's been freed. Here it could be argued that we're just
> before the return so it's pointless, but that would be true if it really
> was the last call before return. In an eyeblink you cannot quickly say
> whether or not the memprintf() call dereferences node (you need to read
> the line). Adding "node = NULL;" just after "free(node)" makes it obvious
> to the reader that this one can be mentally dropped when looking for
> something in the code. In addition, we must always think that many people
> may modify the code, and yhe null is also a safety measure against anyone
> wishing to add it to the memprintf() later.
>
> Thanks!
> Willy
>
From 878cb58e12f62d20e1841c5f01b074368cea1bce Mon Sep 17 00:00:00 2001
From: David Carlier <devne...@gmail.com>
Date: Thu, 21 Sep 2017 14:36:43 +0000
Subject: [PATCH] since we do not log the sample fetch when it is invalid, we
can free the log data.
BUG/MINOR: log: fixing small memory leak in error code path.
since we do not log the sample fetch when it is invalid, we can
free the log data.
---
src/log.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/log.c b/src/log.c
index 5c42f6b2a..520263a71 100644
--- a/src/log.c
+++ b/src/log.c
@@ -484,6 +484,8 @@ int add_sample_to_logformat_list(char *text, char *arg, int arg_len, struct prox
node->options |= LOG_OPT_RES_CAP; /* fetch method is response-compatible */
if (!(expr->fetch->val & cap)) {
+ free(node);
+ node = NULL;
memprintf(err, "sample fetch <%s> may not be reliably used here because it needs '%s' which is not available here",
text, sample_src_names(expr->fetch->use));
return 0;
--
2.14.1
