Hi!

Phew, I was following this one with some concern, fearing it could be something 
more serious just waiting to hit us, too ;-)
Great that the issue was found, thanks for that!

There is just one thing I wanted to note regarding

> […] It can be backported in 1.7, 1.6 and 1.5. I finally marked this patch as 
> a bug fix.

If I read the patch correctly, even though it is classified as “MINOR” it will 
fail with an error on startup, when the configuration has a value outside the 
range.
When backporting into the stable branches, this can lead to updates failing 
with existing configuration files — which may or may not be what you expect 
when doing minor upgrades.
Granted, when you currently use an out-of-range value, you probably _want_ this 
fix, but still might hit you unexpectedly.

It should be made very prominent in the release notes.

Cheers,
Daniel

-- 
Daniel Schneller
Principal Cloud Engineer
 
CenterDevice GmbH                  | Hochstraße 11
                                   | 42697 Solingen
tel: +49 1754155711                | Deutschland
daniel.schnel...@centerdevice.de   | www.centerdevice.de

Geschäftsführung: Dr. Patrick Peschlow, Dr. Lukas Pustina,
Michael Rosbach, Handelsregister-Nr.: HRB 18655,
HR-Gericht: Bonn, USt-IdNr.: DE-815299431


> On 21. Jun. 2017, at 17:00, Christopher Faulet <cfau...@haproxy.com> wrote:
> 
> Le 13/06/2017 à 14:16, Christopher Faulet a écrit :
>> Le 13/06/2017 à 10:31, siclesang a écrit :
>>> haproxy balances by host,but often captures   a part of  request header
>>> host or null, and requests balance to default server.
>>> 
>>> how to debug it ,
>>> 
>> Hi,
>> I'll try to help you. Can you share your configuration please ? It could
>> help to find a potential bug.
>> Could you also provide the tcpdump of a buggy request ?
>> And finally, could you upgrade your HAProxy to the last 1.6 version
>> (1.6.12) to be sure ?
> 
> Hi,
> 
> Just for the record. After some exchanges in private with siclesang, we found 
> the bug in the configuration parser, because of a too high value for 
> tune.http.maxhdr. Here is the explanation:
> 
> Well, I think I found the problem. This is not a bug (not really). There
> is something I missed in your configuration. You set tune.http.maxhdr to
> 64000. I guess you keep this parameter during all your tests. This is an
> invalid value. It needs to be in the range [0, 32767]. This is mandatory
> to avoid integer overflow. the size of the array where headers offsets
> are stored is a signed short.
> 
> To be fair, there is no check on this value during the configuration
> parsing. And the documentation does not specify any range for this
> parameter. I will post a fix very quickly to avoid errors.
> 
> BTW, this is a really huge value. The default one is 101. You can
> legitimately increase this value. But there is no reason to have 64000
> headers in an HTTP message. IMHO, 1000/2000 is already an very huge limit.
> 
> I attached a patch to improve the configuration parsing and to update the 
> documentation. It can be backported in 1.7, 1.6 and 1.5. I finally marked 
> this patch as a bug fix.
> 
> Thanks siclesang for your help,
> -- 
> Christopher Faulet
> <0001-BUG-MINOR-cfgparse-Check-if-tune.http.maxhdr-is-in-t.patch>

Reply via email to