Willy, Lukas,
Am 22.06.20 um 22:10 schrieb Lukas Tribus: > On Mon, 22 Jun 2020 at 21:21, Willy Tarreau <w...@1wt.eu> wrote: >> >> Hi guys, >> >> On Mon, Jun 22, 2020 at 07:49:34PM +0200, Lukas Tribus wrote: >>> Hello Tim, >>> >>> On Mon, 22 Jun 2020 at 18:56, Tim Düsterhus <t...@bastelstu.be> wrote: >>>> >>>> Lukas, >>>> >>>> Am 22.06.20 um 18:41 schrieb Lukas Tribus: >>>>> On Mon, 22 Jun 2020 at 18:16, Tim Duesterhus <t...@bastelstu.be> wrote: >>>>>> >>>>>> Fix parsing of configurations if the configuration file does not end with >>>>>> an LF. >>>>> >>>>> ... but it's also warning about it at the same time. >>>>> >>>>> So it's unclear to me: >>>>> >>>>> Do we support a configuration without trailing LF or not? >>>>> >>>>> If yes, there is no point in a warning (just as < 2.1). >>>>> If no, we should abort and error out. >>>>> >>>>> We can "just warn" if we plan to deprecate it in future release, and >>>>> at that point, explain that fact. But I find it strange to warn about >>>>> something, without a clear indication about *WHY* (unsupported, >>>>> deprecated, etc). >>>>> >>>>> >>>>> Thoughts? >>>>> >>>> I consider leaving out a trailing newline a bug for these reasons: >>>> >>>> [...] >>>> A non-terminated line thus is not a line and handling non-terminated >>>> lines is a bit wonky. >>> >>> What you are explaining is that the behavior is basically undefined, >>> so in my opinion we should just flat out reject this configuration. >>> s/ha_warning/ha_alert ? >>> >>> If we want to continue with a warning only (to not break older >>> configs), let's elaborate, because the warning just explains that the >>> line is not terminated, but not if and why that is actually a problem, >>> which I don't like to leave as an exercise for the reader (user). >>> >>> ha_warning("parsing [%s:%d]: line is not terminated by a newline (LF / >>> '\\n'), this may lead to undefined behavior.\n", >>> >>> ha_warning("parsing [%s:%d]: line should be terminated by a newline >>> (LF / '\\n'), otherwise undefined behavior may occur.\n", >>> >>> Something like that? >>> >>> >>> But really, I think we should just reject this instead (then the text >>> suffices, because it actually stops working). >> >> It used to work and certainly happens from time to time to people who >> generate their own configs. It's too easy to concatenate pieces of >> strings and not have the final condition ready to emit the last LF. >> For example if you emit you cookie options based on various tests, >> you may end up appending words without the trailing "\n" and decide >> to emit one at the beginning of each line instead. I've seen such >> configs from time to time so I know they do happen. >> >> I think I could be fine with dropping support for this (unless somebody >> objects here), but not in the LTS branch and not without a prior warning, >> so that users have time to fix their scripts. >> >> Also if we change this we have to update the doc to mention that all >> lines must be terminated. >> >> It's not uncommon to see files missing the last LF, and Git even has a >> warning for this. But the main issue to be reported there is that the >> file might have accidently been truncated (e.g. file-system full during >> the copy). However I agree with you Lukas that the warning should >> clearly indicate the impact and what needs to be fixed. >> >> But it gets tricky. There's a known flaw of fgets() making it return an >> incomplete line: if a \0 is present on the line before the \n. And given >> that fgets returns a pointer to a zero-terminated string instead of a >> length, the only way to read the string is to read it up to \0. So a >> line not ending with \n is not necessarily a truncated one but might be >> one with a \0 anywhere in the middle. >> >> So what we could do if we want to do something clean is the following: >> - detect \0 or truncation on a line and note its position ; >> - if we find another line once a truncated line has been found, we >> emit a warning about "stray NUL character at position X on line Y, >> this will not be supported in 2.3 anymore" and reset this position. >> - at the end of the loop, if the last NUL position is still set, we >> emit a warning about "missing LF on last line, file might have been >> truncated, this will not be supported in 2.3 anymore". >> >> And in 2.3 we turn that into errors. >> >> What do you guys think about it ? > > I like it, yes. > > > Lukas > printf "listen foo\nbind *:8080\x00test\nmode http\x00stuff\nlog global\x00bad\n" Tim Düsterhus (2): BUG/MINOR: cfgparse: Support configurations without newline at EOF MINOR: cfgparse: Warn on truncated lines / files src/cfgparse.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) -- 2.27.0