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 ?
Willy
