BUG/MEDIUM: channel: fix inconsistent handling of 4GB-1 transfers

In 1.4-dev3, commit 31971e5 ("[MEDIUM] add support for infinite forwarding")
made it possible to configure the lower layer to forward data indefinitely
by setting the forward size to CHN_INFINITE_FORWARD (4GB-1). By then larger
chunk sizes were not supported so there was no confusion in the usage of the
function.

Since 1.5 we support 64-bit content-lengths and chunk sizes and the function
has grown to support 64-bit arguments, though it still limits a single pass
to 32-bit quantities (what fit in the channel's to_forward field). The issue
now becomes that a 4GB-1 content-length can be confused with infinite
forwarding (in fact it's 4GB-1+what was already in the buffer). It causes a
visible effect when transferring this exact size because the transfer rate
is lower than with other sizes due in part to the disabling of the Nagle
algorithm on the sendto() call.

In theory with keep-alive it should prevent a second request from being
processed after such a transfer, but since the analysers are still present,
the forwarding analyser properly counts down the remaining size to transfer
and ultimately the transaction gets correctly reset so there is no visible
effect.

Since the root cause of the issue is an API problem (lack of distinction
between a real valid length and a magic value), this patch modifies the API
to have a new dedicated function called channel_forward_forever() to program
a permanent forwarding. The existing function __channel_forward() was modified
to properly take care of the requested sizes and ensure it 1) never overflows
and 2) never reaches CHN_INFINITE_FORWARD by accident.

It is worth noting that the function used to have a bug causing a 2GB
forward to be scheduled if it was called with less data than what is present
in buf->i. Fortunately this bug couldn't be triggered with existing code.

This fix should be backported to 1.6 and 1.5. While it also theorically
affects 1.4, it's better not to backport it there, as the risk of breaking
large object transfers due to significant API differences is high, compared
to the fact that the largest supported objects (4GB-1) are just slower to
transfer.

diff --git a/include/proto/channel.h b/include/proto/channel.h
index cbc136cc56cf9fad8308f20048863a3e50823e37..3d435c47ce94a8b9002a8abf734fa246fee69f36 100644 (file)
--- a/include/proto/channel.h
+++ b/include/proto/channel.h
@@ -120,6 +120,13 @@ static inline unsigned long long channel_forward(struct channel *chn, unsigned l
        return __channel_forward(chn, bytes);
 }
 
+/* Forwards any input data and marks the channel for permanent forwarding */
+static inline void channel_forward_forever(struct channel *chn)
+{
+       b_adv(chn->buf, chn->buf->i);
+       chn->to_forward = CHN_INFINITE_FORWARD;
+}
+
 /*********************************************************************/
 /* These functions are used to compute various channel content sizes */
 /*********************************************************************/

diff --git a/src/channel.c b/src/channel.c
index 4728986cba0b368647b3d11e89d056f6f380381d..46921b856dc73a949bc1795ed191ca3012a441ca 100644 (file)
--- a/src/channel.c
+++ b/src/channel.c
@@ -23,44 +23,49 @@
 
 /* Schedule up to <bytes> more bytes to be forwarded via the channel without
  * notifying the owner task. Any data pending in the buffer are scheduled to be
- * sent as well, in the limit of the number of bytes to forward. This must be
- * the only method to use to schedule bytes to be forwarded. If the requested
+ * sent as well, within the limit of the number of bytes to forward. This must
+ * be the only method to use to schedule bytes to be forwarded. If the requested
  * number is too large, it is automatically adjusted. The number of bytes taken
  * into account is returned. Directly touching ->to_forward will cause lockups
  * when buf->o goes down to zero if nobody is ready to push the remaining data.
  */
 unsigned long long __channel_forward(struct channel *chn, unsigned long long bytes)
 {
-       unsigned int new_forward;
+       unsigned int budget;
        unsigned int forwarded;
 
-       forwarded = chn->buf->i;
-       b_adv(chn->buf, chn->buf->i);
-
-       /* Note: the case below is the only case where we may return
-        * a byte count that does not fit into a 32-bit number.
+       /* This is more of a safety measure as it's not supposed to happen in
+        * regular code paths.
         */
-       if (likely(chn->to_forward == CHN_INFINITE_FORWARD))
-               return bytes;
-
-       if (likely(bytes == CHN_INFINITE_FORWARD)) {
-               chn->to_forward = bytes;
+       if (unlikely(chn->to_forward == CHN_INFINITE_FORWARD)) {
+               b_adv(chn->buf, chn->buf->i);
                return bytes;
        }
 
-       new_forward = chn->to_forward + bytes - forwarded;
-       bytes = forwarded; /* at least those bytes were scheduled */
+       /* Bound the transferred size to a 32-bit count since all our values
+        * are 32-bit, and we don't want to reach CHN_INFINITE_FORWARD.
+        */
+       budget = MIN(bytes, CHN_INFINITE_FORWARD - 1);
+
+       /* transfer as much as we can of buf->i */
+       forwarded = MIN(chn->buf->i, budget);
+       b_adv(chn->buf, forwarded);
+       budget -= forwarded;
 
-       if (new_forward <= chn->to_forward) {
-               /* integer overflow detected, let's assume no more than 2G at once */
-               new_forward = MID_RANGE(new_forward);
-       }
+       if (!budget)
+               return forwarded;
 
-       if (new_forward > chn->to_forward) {
-               bytes += new_forward - chn->to_forward;
-               chn->to_forward = new_forward;
-       }
-       return bytes;
+       /* Now we must ensure chn->to_forward sats below CHN_INFINITE_FORWARD,
+        * which also implies it won't overflow. It's less operations in 64-bit.
+        */
+       bytes = (unsigned long long)chn->to_forward + budget;
+       if (bytes >= CHN_INFINITE_FORWARD)
+               bytes = CHN_INFINITE_FORWARD - 1;
+       budget = bytes - chn->to_forward;
+
+       chn->to_forward += budget;
+       forwarded += budget;
+       return forwarded;
 }
 
 /* writes <len> bytes from message <msg> to the channel's buffer. Returns -1 in

diff --git a/src/stream.c b/src/stream.c
index da707550b95cce6d620bd8f840b974ce06faf935..852328924b2ac0b15428dad1525416b0ce069587 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -2156,7 +2156,7 @@ struct task *process_stream(struct task *t)
                 * to the consumer (which might possibly not be connected yet).
                 */
                if (!(req->flags & (CF_SHUTR|CF_SHUTW_NOW)))
-                       channel_forward(req, CHN_INFINITE_FORWARD);
+                       channel_forward_forever(req);
 
                /* Just in order to support fetching HTTP contents after start
                 * of forwarding when the HTTP forwarding analyser is not used,
@@ -2318,7 +2318,7 @@ struct task *process_stream(struct task *t)
                 * to the consumer.
                 */
                if (!(res->flags & (CF_SHUTR|CF_SHUTW_NOW)))
-                       channel_forward(res, CHN_INFINITE_FORWARD);
+                       channel_forward_forever(res);
 
                /* Just in order to support fetching HTTP contents after start
                 * of forwarding when the HTTP forwarding analyser is not used,