Hi Godbach,
[ and first, sorry for not having yet responded to your
other mail about caching ]
On Thu, Oct 30, 2014 at 05:07:15PM +0800, Godbach wrote:
> Hi Willy,
>
> I have test both haproxy-1.5 and latest snapshot. HAProxy will crash
> with the following configuration:
>
> global
> ...
> tune.bufsize 1024
> tune.maxrewrite 0
> frontend xxx
> ...
> backend yyy
> ...
> cookie cookie insert maxidle 300s
>
> If client sends a request of which object size is more than tune.bufsize
> (1024 bytes), HAProxy will crash.
Argh, that's really bad!
> After doing some debugging, the crash was caused by
> http_header_add_tail2() -> buffer_insert_line2() while inserting cookie
> at the end of response header. Part codes of buffer_insert_line2() are
> as below:
>
> int buffer_insert_line2(struct buffer *b, char *pos, const char *str,
> int len)
> {
> int delta;
>
> delta = len + 2;
>
> if (bi_end(b) + delta >= b->data + b->size)
> return 0; /* no space left */
>
> /* first, protect the end of the buffer */
> memmove(pos + delta, pos, bi_end(b) - pos);
> ...
> }
>
> If HAProxy received 1024 bytes which is equals to full buffer size since
> maxrewrite is 0, the buffer should be full and bi_end(b) will be wrapped
> to the start of buffer which pointed to b->data.
That's the bad thing indeed. Ideally we'd have a bi_left() function which
computes how much remains past b->ptr+b->i (sort of a simplified version
of buffer_contig_space) and that would avoid such stupid wrapping issues.
> As a result, though there is no space in buffer, the check condition
>
> if (bi_end(b) + delta >= b->data + b->size)
> will be true, then memmove() is called, and pos + delta will exceed
> b->data + b->size, HAProxy crashes.
>
> In my opinion, this bug will affect all the rewrite actions which will
> add new header to the end of current http header.
Absolutely.
> Just take buffer_replace2() as a reference, the other check should be
> added into buffer_insert_line2() as below:
>
> diff --git a/src/buffer.c b/src/buffer.c
> index 91bee63..9037dd3 100644
> --- a/src/buffer.c
> +++ b/src/buffer.c
> @@ -88,6 +88,11 @@ int buffer_insert_line2(struct buffer *b, char *pos,
> const char *str, int len)
> if (bi_end(b) + delta >= b->data + b->size)
> return 0; /* no space left */
>
> + if (buffer_not_empty(b) &&
> + bi_end(b) + delta > bo_ptr(b) &&
> + bo_ptr(b) >= bi_end(b))
> + return 0; /* no space left before wrapping data */
> +
> /* first, protect the end of the buffer */
> memmove(pos + delta, pos, bi_end(b) - pos);
>
> This patch can be work well for both 1.5 release and 1.6-dev. Please
> help review this patch and I will send a patch if it is OK.
Yes, it looks fine to me, feel free to send a patch and tag it as BUG/MAJOR.
In the future we'll probably need to rework all these functions, they have
lived a lot since version 1.1...
Thanks!
Willy
