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.
(cherry picked from commit ef907fee12b086c8c95f726579b95f09af3b33c5)
(cherry picked from commit 8cfd96049a0bae351b168c3b0c46feae2e0b263b)
diff --git a/include/proto/channel.h b/include/proto/channel.h
index 189be86..2d3c9bc 100644
--- a/include/proto/channel.h
+++ b/include/proto/channel.h
@@ -346,6 +346,7 @@
*/
static inline int buffer_max_len(const struct channel *chn)
{
+ unsigned int transit;
int reserve;
/* return size - maxrewrite if we can't send */
@@ -353,12 +354,19 @@
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;