Re: Truncated health check response from real servers

Hi Anze,

On Fri, Jul 09, 2010 at 01:48:12AM +0200, Anze wrote:
>
> > Indeed it never made it in. Each time I look at it, I realize that
> > cleaning it up and fixing it will take more time than what I have
> > available to work on it :-/
>
> I can understand that completely. :)
>
> Actually, I am not that much interested in the patch per-se, I would just like
> to check the body of the check-response and compare it to a string for more
> reliable checks.

It's what the patch does.

> I have included my original mail (and reasoning) on the
> subject below. It's not on top of my priority list right now either, still...
> sooner or later we might get bitten by it.
>
> I'd love to help out coding, but sockets and C++ are not my strong points
> (though I am comfortable in C).

So you're welcome, haproxy is plain C, and I don't understand C++ either ;-)

> I can help with testing though if needed. Let
> me know if it helps and if/when you find time for this. :)
>
> Or, if you wish, I can take Nick's patch and extract just the parts with
> checking the response body? Can you point out some of the errors so I can
> avoid them?

I've rediffed the patch against 1.4.8, you can find it here :

    http://haproxy.1wt.eu/download/patches/http-ecv-checks-1.4.8.diff

Most of the things I'm particularly concerned are in httpchk_expect() :

• multiple calls to strstr(), strncmp() or regexec() on a non-null terminated string. It only works if the buffer contained a null from a previous use, otherwise it will happily overflow the buffer ! Either we find how to properly bound the functions, though I'm not sure we can for regexec(), or we force a '\0' at the end of the data received from the server, possibly truncating the response by one byte if the buffer is full.
• a calloc() call inside the check code ! We don't want to perform dynamic memory allocation outside of the pools once the code is running. Also, the return is not checked. The proper way to replace this is to use trash[] as a temporary variable, it's designed for this.
• working mode checked with strcmp() ! This is particularly slow and complicated, I'd rather have enums that are correctly set when the config is parsed, and make use of them normally with a swich/case of even an if statement.
• one or a few inline variable declarations which will break build on older compilers still in use in some embedded systems (easily fixed though).

As you see, there's not even any socket work on this, so if you feel comfortable with this, feel free to contribute, I think this is the patch that received the most different contributors' work.

Once it's OK, I'm willing to merge it into next 1.4 stable because I know that several people would like to use it, and it doesn't affect other parts of code, so there's no risk of regression if it breaks.

Thanks,
Willy Received on 2010/07/10 10:06