I attached only the first patch at a time. ср, 15 июл. 2020 г. в 13:08, Илья Шипицин <[email protected]>:
> > > ср, 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 >> >
From 8c231a2a3b776a623f64b9fe496b3667a4af4bfb Mon Sep 17 00:00:00 2001 From: Ilya Shipitsin <[email protected]> Date: Thu, 16 Jul 2020 02:02:40 +0500 Subject: [PATCH] BUG/MEDIUM: src/server.c: resolve file handle leak on reload during reload, server state file is read and file handle is not released this was indepently reported in #738 and #660 partially resolves #660 --- src/server.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/server.c b/src/server.c index 05b19d4e1..bd3dcf886 100644 --- a/src/server.c +++ b/src/server.c @@ -3268,6 +3268,11 @@ void apply_server_state(void) } out_load_server_state_in_tree: + if (f) { + fclose(f); + f = NULL; + } + /* parse all proxies and load states form tree (global file) or from local file */ for (curproxy = proxies_list; curproxy != NULL; curproxy = curproxy->next) { /* servers are only in backends */ @@ -3447,9 +3452,11 @@ 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); } /* now free memory allocated for the tree */ -- 2.26.2
