Hi Ilya,
> From 4f62799eba5db5fe6400d458877677f098da3b13 Mon Sep 17 00:00:00 2001
> From: Ilya Shipitsin <[email protected]>
> Date: Sun, 12 Jul 2020 15:24:55 +0500
> Subject: [PATCH] src/server.c: add extra guards when loading state file
>
> this should fix #660
>
> we can only load 'local' state file, if file is corrupted,
> we should check filepath against NULL
>
> also, global server state file should be properly closed upon read.
> otherwise, it might leak on haproxy reload.
>
> when reading global server state file we should check string lenght,
> if file was corrupted we might observe overflow otherwise.
So do I understand it right that there are actually two unrelated, or
maybe even three bugs there ? If so, please always take care of splitting
them (one fix per bug) so that they could be independently reviewed and
backported as needed (or sometimes possibly reverted if the fix causes
unexpected breakage).
> ---
> src/server.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/server.c b/src/server.c
> index 05b19d4e1..54239c63c 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -3265,6 +3265,7 @@ void apply_server_state(void)
> /* free up memory in case of error during the
> processing of the line */
> free(line);
> }
> + fclose(f);
> }
> out_load_server_state_in_tree:
I had a look at the patch in its context and this will still leak in the
case where global_file_version == 0.
The right way to do it would instead to add this after this label
"out_load_server_state_in_tree" :
if (f) {
fclose(f);
f = NULL;
}
> @@ -3360,6 +3361,8 @@ void apply_server_state(void)
> goto next;
>
> st = container_of(node, struct state_line,
> name_name);
> + if (strlen(st->line) > SRV_STATE_LINE_MAXLEN)
> + goto next;
So from what I'm seeing elsewhere in the code, this test will never match:
a line is read using fgets() with a size of SRV_STATE_LINE_MAXLEN (hence it
will always read one less char), and it is passed to srv_state_parse_line()
which returns an error when the '\n' is missing, hence the line is skipped
and never indexed, so this cannot happen at all.
> memcpy(mybuf, st->line, strlen(st->line));
> mybuf[strlen(st->line)] = 0;
>
> @@ -3375,7 +3378,7 @@ void apply_server_state(void)
>
> continue; /* next proxy in list */
> }
> - else {
> + else if (filepath) {
Is it a third issue ? The whole code is hard to follow for me but what is
the exact case you've tried to address here ? Looking through the code, it
looks like we could land there when the local file name is too large (but
apparently there is no error handling for this where this path name is
constructed) :-/
> /* load 'local' state file */
> errno = 0;
> f = fopen(filepath, "r");
> @@ -3447,9 +3450,9 @@ void apply_server_state(void)
> /* now we can proceed with server's state
> update */
> srv_update_state(srv, version, srv_params);
> }
> + fileclose:
> + fclose(f);
> }
> -fileclose:
> - fclose(f);
> }
I agree, this one definitely makes sense!
Thanks!
Willy
