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.
diff --git a/include/proto/channel.h b/include/proto/channel.h
index 22f28fc..cbc136c 100644
--- a/include/proto/channel.h
+++ b/include/proto/channel.h
@@ -386,6 +386,7 @@
*/
static inline int channel_recv_limit(const struct channel *chn)
{
+ unsigned int transit;
int reserve;
/* return zero if empty */
@@ -398,12 +399,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;