From ef907fee12b086c8c95f726579b95f09af3b33c5 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 3 May 2016 17:46:24 +0200 Subject: [PATCH] BUG/MAJOR: channel: fix miscalculation of available buffer space (4th try) Unfortunately, commit 169c470 ("BUG/MEDIUM: channel: fix miscalculation of available buffer space (3rd try)") was still not enough to completely address the issue. It fell into an integer comparison trap. Contrary to expectations, chn->to_forward may also have the sign bit set when forwarding regular data having a large content-length, resulting in an incomplete check of the result and of the reserve because the with to_forward very large, to_forward+o could become very small and also the reserve could become positive again and make channel_recv_limit() return a negative value. One way to reproduce this situation is to transfer a large file (> 2GB) with http-keep-alive or http-server-close, without splicing, and ensure that the server uses content-length instead of chunks. The transfer should stall very early after the first buffer has been transferred to the client. This fix now properly checks 1) for an overflow caused by summing o and to_forward, and 2) for o+to_forward being smaller or larger than maxrw before performing the subtract, so that all sensitive operations are properly performed on 33-bit arithmetics. The code was subjected again to a series of tests using inject+httpterm scanning a wide range of object sizes (+10MB after each new request) : $ printf "new page 1\nget 127.0.0.1:8002 / s=%%s0m\n" | \ inject64 -o 1 -u 1 -f /dev/stdin With previous fix, the transfer would suddenly stop when reaching 2GB : hits ^hits hits/s ^h/s bytes kB/s last errs tout htime sdht ptime 203 1 2 1 216816173354 2710202 3144892 0 0 685.0 0.0 685.0 205 2 2 2 219257283186 2706880 2441109 0 0 679.5 6.5 679.5 205 0 2 0 219257283186 2673836 0 0 0 0.0 0.0 0.0 205 0 2 0 219257283186 2641622 0 0 0 0.0 0.0 0.0 205 0 2 0 219257283186 2610174 0 0 0 0.0 0.0 0.0 Now it's fine even past 4 GB. Many thanks to Vedran Furac for reporting this issue early with a common access pattern helping to troubleshoot this. This fix must be backported to 1.6 and 1.5 where the commit above was already backported. --- include/proto/channel.h | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/include/proto/channel.h b/include/proto/channel.h index 22f28fcc1..cbc136cc5 100644 --- a/include/proto/channel.h +++ b/include/proto/channel.h @@ -386,6 +386,7 @@ static inline void channel_dont_read(struct channel *chn) */ static inline int channel_recv_limit(const struct channel *chn) { + unsigned int transit; int reserve; /* return zero if empty */ @@ -398,12 +399,19 @@ static inline int channel_recv_limit(const struct channel *chn) if (unlikely(!channel_may_send(chn))) goto end; - /* chn->to_forward may cause an integer underflow when equal to - * CHN_INFINITE_FORWARD but we check for it afterwards as it produces - * quite better assembly code in this sensitive code path. + /* We need to check what remains of the reserve after o and to_forward + * have been transmitted, but they can overflow together and they can + * cause an integer underflow in the comparison since both are unsigned + * while maxrewrite is signed. + * The code below has been verified for being a valid check for this : + * - if (o + to_forward) overflow => return size [ large enough ] + * - if o + to_forward >= maxrw => return size [ large enough ] + * - otherwise return size - (maxrw - (o + to_forward)) */ - reserve = global.tune.maxrewrite - chn->buf->o - chn->to_forward; - if (chn->to_forward == CHN_INFINITE_FORWARD || reserve < 0) + transit = chn->buf->o + chn->to_forward; + reserve -= transit; + if (transit < chn->to_forward || // addition overflow + transit >= (unsigned)global.tune.maxrewrite) // enough transit data return chn->buf->size; end: return chn->buf->size - reserve;