Willy,
Lukas,
Am 22.06.20 um 22:10 schrieb Lukas Tribus:
> On Mon, 22 Jun 2020 at 21:21, Willy Tarreau 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 wrote:
Lukas,
Am 22.06.20 um 18:41 schrieb Lukas Tribus:
> On Mon, 22 Jun 2020 at 18:16, Tim Duesterhus 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
