Re: [PATCH] Handle long lines properly

Hi Krzysztof,

On Sun, Oct 28, 2007 at 07:58:30PM +0100, Krzysztof Oledzki wrote:
> >From 1a487fb52b94e3ab2f2ced90ee85c5ff21e51959 Mon Sep 17 00:00:00 2001
> From: Krzysztof Piotr Oledzki <ole#ans.pl>
> Date: Sun, 28 Oct 2007 17:09:08 +0100
> Subject: [MEDIUM] Handle long lines properly
>
> Currently, there is a hidden line length limit in the haproxy, set
> to 256-1 chars. With large acls (for example many hdr(host) matches)
> it may be not enough and which is even worse, error message may
> be totally confusing as everything above this limit is treaded
> as a next line:
>
> echo -ne "frontend aqq 1.2.3.4:80\nmode http\nacl e hdr(host) -i X X X X X X X www.xx.example.com stats\n"|
> sed s/X/www.some-host-name.example.com/g > ha.cfg && haproxy -c -f ./ha.cfg
>
> [WARNING] 300/163906 (11342) : parsing [./ha.cfg:4] : 'stats' ignored because frontend 'aqq' has no backend capability.
>
> Yesterday I hit simmilar problem and it took me a while to find why
> requests for "stats" are not handled properly.

Yes, I was worrying that such problem would finally happen. I sometimes even wonder if the MAX_LINE_ARGS currently set to 40 will always be enough. My conclusion is that too long lines are unreadable and should be broken into subsections. Eg: servers may very well be accepted on multiple lines. But that's not for tomorrow :-)

> This patch:
> - makes the limit configurable
> - increases the default value from 256 to 2048
> - adds the code that shows error if the limit is reached
> - changes "*line++ = 0;" to "*line++ = '\0';" (cosmetics)
>
> With this patch, when LINELENGTH is defined to 256, above example produces:
> [ALERT] 300/164724 (27364) : parsing [/tmp/ha.cfg:3]: line too long, limit: 255.
> [ALERT] 300/164724 (27364) : Error reading configuration file : /tmp/ha.cfg

Seems better to me!

> --- a/include/common/defaults.h
> +++ b/include/common/defaults.h
> @@ -35,6 +35,10 @@
> #define BUFSIZE 16384
> #endif
>
> +#ifndef LINESIZE
> +#define LINESIZE 2048
> +#endif

Could you please move this just above MAX_LINE_ARGS, a few lines below, and maybe at the same time increase MAX_LINE_ARGS ?

> @@ -2323,6 +2323,15 @@ int readcfgfile(const char *file)
>
> end = line + strlen(line);
>
> + if (*(end-1) != '\n' && strlen(line) + 1 == sizeof(thisline)) {
> + /* Check if the last char is not \n and we reached the limit. Watch out
> + * for the last line without the terminating '\n'!
> + */

I think that the second part of the check is needed, as it seems to me that the only case where we can encounter something other than '\n' in *(end-1) is precisely when the line has been truncated.

Otherwise OK for me.

Cheers,
Willy Received on 2007/10/29 00:05