Hi Dirkjan,
On Mon, Apr 28, 2014 at 04:00:18PM -0700, Dirkjan Bussink wrote:
> Hi all,
>
> When building HAProxy using the Clang Static Analyzer, it found a few cases
> of invalid memory usage and leaks. I?ve attached a patch to fix these cases.
I think there are 3 types of errors fixed by your patch :
- bugs that occur at runtime (eg: pat_ref_delete()).
- use-after-free in the error path, which can cause hard-to-diagnose
crashes when config errors are detected ;
- leaks in the error path which are harmless since the process is exiting
anyway. However I agree to take them as cleanups.
That said, one of your fix introduces a bug here :
diff --git a/src/haproxy.c b/src/haproxy.c
index ed2ff21..c1ec783 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -1607,6 +1607,7 @@ int main(int argc, char **argv)
exit(0); /* parent must leave */
}
+ free(children);
/* if we're NOT in QUIET mode, we should now close the 3 first
FDs to ensure
* that we can detach from the TTY. We MUST NOT do it in other
cases since
* it would have already be done, and 0-2 would have been
affected to listening
Indeed, children is used a few lines below :
if (proc == global.nbproc) {
if (global.mode & MODE_SYSTEMD) {
for (proc = 0; proc < global.nbproc; proc++)
while (waitpid(children[proc], NULL, 0)
== -1 && errno == EINTR);
}
exit(0); /* parent must leave */
}
In order to avoid getting trapped by such risks of use-after-free, I strongly
suggest that you assign a pointer to NULL after freeing it whenever relevant.
It ensures that such cases are detected very early.
Last, as Dmitri mentionned it, please do not add a test for the pointer to be
freed. free(foo) is fine even if foo is null.
Would you mind retransmitting the fixed patch or do you prefer me to fix it
while applying it ?
Thanks,
Willy
