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
