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
