ср, 15 июл. 2020 г. в 12:00, Willy Tarreau <[email protected]>: > 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! >
I will address comments today or tomorrow > > Thanks! > Willy >
