On Tue, Nov 28, 2017 at 11:58:50PM +0100, PiBa-NL wrote:
> Hi List, Willy / Willliam,
>
Hi Pieter,
> A patch i came up with that might make it a little 'safer' with regard
> to getenv and its return value or possible lack thereof.. I'm not sure
> it it will ever happen. But if it does it wont fail on a null pointer or
> empty string conversion to a long value.. Though a arithmetic conversion
> error could still happen if the value is present but not a number..but
> well that would be a really odd case.
>
The getenv returning NULL should never happen, but the test is wrong, it should
have been a strtol with an errno check instead of an atol. However that's
overkill in this case, we just need to check the return value of getenv().
> There are a few things i'm not sure about though.
>
> - What would/could possibly break if mworker_pipe values are left as -1
> and the process continues and tries to use it?
That does not seems to be a good idea, because we try to register the fd in the
poller after that. We don't need to do this, it's better to quit the
master-worker if something this simple failed, because we can't trust the
process anymore.
> - wont the rd wr char* values leak memory?
No because you don't need to free the return value of a getenv, it's a pointer
to the environment.
> Anyhow the biggest part that should be noticed of the bug is the
> sometimes wrongful alert when the fd is actually '0'...
>
That's right, the check was not coherent there, we should check only if
getenv return NULL, if the env was modified between the master or the worker
there is a big problem on the system, so no need to check the converted value.
> If anything needs to be changed let me know.
>
> Regards,
>
> PiBa-NL / Pieter
>
>
> From 486d7c759af03f9193ae3e38005d8325ab069b37 Mon Sep 17 00:00:00 2001
> From: PiBa-NL <pba_...@yahoo.com>
> Date: Tue, 28 Nov 2017 23:22:14 +0100
> Subject: [PATCH] [PATCH] BUG/MINOR: Check if master-worker pipe getenv
> succeeded, also allow pipe fd 0 as valid.
>
> On FreeBSD in quiet mode the stdin/stdout/stderr are closed which lets the
> mworker_pipe to use fd 0 and fd 1.
> ---
> src/haproxy.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/haproxy.c b/src/haproxy.c
> index 891a021..c3c8281 100644
> --- a/src/haproxy.c
> +++ b/src/haproxy.c
> @@ -2688,9 +2688,15 @@ int main(int argc, char **argv)
> free(msg);
> }
> } else {
> - mworker_pipe[0] =
> atol(getenv("HAPROXY_MWORKER_PIPE_RD"));
> - mworker_pipe[1] =
> atol(getenv("HAPROXY_MWORKER_PIPE_WR"));
> - if (mworker_pipe[0] <= 0 || mworker_pipe[1] <=
> 0) {
> + mworker_pipe[0] = -1;
> + mworker_pipe[1] = -1;
We don't need to init to -1;
> + char* rd = getenv("HAPROXY_MWORKER_PIPE_RD");
> + char* wr = getenv("HAPROXY_MWORKER_PIPE_WR");
In my opinion we can simplify:
> + if (rd && wr && strlen(rd) > 0 && strlen(wr) >
> 0) {
> + mworker_pipe[0] = atol(rd);
> + mworker_pipe[1] = atol(wr);
> + }
> + if (mworker_pipe[0] < 0 || mworker_pipe[1] < 0)
> {
> ha_warning("[%s.main()] Cannot get
> master pipe FDs.\n", argv[0]);
> }
By doing this, which is more secure, and assure us that it won't start with
this kind of problem:
if (!rd || !wd) {
ha_alert("[%s.main()] Cannot get master
pipe FDs.\n", argv[0]);
exit(1);
}
mworker_pipe[0] = atoi(rd);
mworker_pipe[1] = atoi(wr);
And we can do the same thing with the pipe return value:
ret = pipe(mworker_pipe);
if (ret < 0) {
ha_alert("[%s.main()] Cannot create
master pipe.\n", argv[0]);
exit(1);
} ....
This code will guarantee that the whole master-worker quit if there is a
problem.
--
William Lallemand
