Re: [PATCH] BUG/MINOR: cfgparse: Support configurations without newline at EOF
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
Re: [PATCH] BUG/MINOR: cfgparse: Support configurations without newline at EOF
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 ?
Willy
Re: [PATCH] BUG/MINOR: cfgparse: Support configurations without newline at EOF
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).
--
lukas
Re: [PATCH] BUG/MINOR: cfgparse: Support configurations without newline at EOF
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: 1. POSIX defines a line as "A sequence of zero or more non- characters plus a terminating character." (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206). A non-terminated line thus is not a line and handling non-terminated lines is a bit wonky. 2. Leaving out the trailing newline causes ./haproxy -f config/ with config/ being a folder not to be equivalent to ./haproxy -f config with config being the catenation of all files of the config/ folder (⏎ indicates a missing trailing newline in my shell): > $ ls -alh > total 84K > drwxrwxr-x 2 timwolla timwolla 4.0K Jun 22 18:47 . > drwxrwxrwt 29 root root 68K Jun 22 18:47 .. > -rw-rw-r-- 1 timwolla timwolla 22 Jun 22 18:47 a.cfg > -rw-rw-r-- 1 timwolla timwolla 23 Jun 22 18:47 b.cfg > $ cat a.cfg > listen foo > bind *:8080⏎> $ cat b.cfg > listen foo2 > bind *:8081⏎> $ cat a.cfg b.cfg > cat Then: > $ ./haproxy -d -f /tmp/example/ > [WARNING] 173/185127 (17393) : parsing [/tmp/example//a.cfg:2]: line is not > terminated by a newline (LF / '\n'). > [WARNING] 173/185127 (17393) : parsing [/tmp/example//b.cfg:2]: line is not > terminated by a newline (LF / '\n'). > [WARNING] 173/185127 (17393) : config : missing timeouts for proxy 'foo'. >| While not properly invalid, you will certainly encounter various problems >| with such a configuration. To fix this, please ensure that all following >| timeouts are set to a non-zero value: 'client', 'connect', 'server'. > [WARNING] 173/185127 (17393) : config : missing timeouts for proxy 'foo2'. >| While not properly invalid, you will certainly encounter various problems >| with such a configuration. To fix this, please ensure that all following >| timeouts are set to a non-zero value: 'client', 'connect', 'server'. > Note: setting global.maxconn to 524276. > Available polling systems : > epoll : pref=300, test result OK >poll : pref=200, test result OK > select : pref=150, test result FAILED > Total: 3 (2 usable), will use epoll. > > Available filters : > [SPOE] spoe > [COMP] compression > [TRACE] trace > [CACHE] cache > [FCGI] fcgi-app > Using epoll() as the polling mechanism. > ^C⏎ > $ ./haproxy -d -f /tmp/example/cat > [NOTICE] 173/185130 (17415) : haproxy version is 2.2-dev10-55729a-1 > [NOTICE] 173/185130 (17415) : path to executable is ./haproxy > [ALERT] 173/185130 (17415) : parsing [/tmp/example/cat:2] : 'bind > *:8080listen' unknown keyword 'foo2'. Registered keywords : > [STAT] level > [STAT] expose-fd > [STAT] severity-output > [ ALL] accept-netscaler-cip > [ ALL] accept-proxy > [ ALL] backlog > [ ALL] id > [ ALL] maxconn > [ ALL] name > [ ALL] nice > [ ALL] process > [ ALL] proto > [UNIX] gid > [UNIX] group > [UNIX] mode > [UNIX] uid > [UNIX] user > [ TCP] defer-accept > [ TCP] interface > [ TCP] mss > [ TCP] tcp-ut > [ TCP] tfo > [ TCP] transparent > [ TCP] v4v6 > [ TCP] v6only > [ TCP] namespace > [WARNING] 173/185130 (17415) : parsing [/tmp/example/cat:3]: line is not > terminated by a newline (LF / '\n'). > [ALERT] 173/185130 (17415) : Error(s) found in configuration file : > /tmp/example/cat > [ALERT] 173/185130 (17415) : Fatal errors found in configuration. Thus even if we might never not support leaving out the trailing newline I consider that something worthwhile to warn about. Best regards Tim Düsterhus
Re: [PATCH] BUG/MINOR: cfgparse: Support configurations without newline at EOF
Hello, 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? --lukas
