BUG/MEDIUM: peers: fix decoding of multi-byte length in stick-table messages

There is a bug in peer_recv_msg() due to an incorrect cast when trying
to decode the varint length of a stick-table message, causing lengths
comprised between 128 and 255 to consume one extra byte, ending in
protocol errors.  The root cause of this is that peer_recv_msg() tries
hard to reimplement all the parsing and control that is already done in
intdecode() just to measure the length before calling it. And it got it
wrong.

Let's just get rid of this unneeded code duplication and solely rely on
intdecode() instead. The bug was introduced in 2.0 as part of a cleanup
pass on this code with commit 95203f218 ("MINOR: peers: Move high level
receive code to reduce the size of I/O handler."), so this patch must
be backported to 2.0.

Thanks to Yves Lafon for reporting the problem.

diff --git a/src/peers.c b/src/peers.c
index 6dbc3a964984788e15daa469487e30d9e7b9f734..3466fa576217e0befbf73f6ecc429b9126cfa30b 100644 (file)
--- a/src/peers.c
+++ b/src/peers.c
@@ -1826,7 +1826,14 @@ static inline int peer_treat_definemsg(struct appctx *appctx, struct peer *p,
 }
 
 /*
- * Receive a stick-table message.
+ * Receive a stick-table message or pre-parse any other message.
+ * The message's header will be sent into <msg_head> which must be at least
+ * <msg_head_sz> bytes long (at least 7 to store 32-bit variable lengths).
+ * The first two bytes are always read, and the rest is only read if the
+ * first bytes indicate a stick-table message. If the message is a stick-table
+ * message, the varint is decoded and the equivalent number of bytes will be
+ * copied into the trash at trash.area. <totl> is incremented by the number of
+ * bytes read EVEN IN CASE OF INCOMPLETE MESSAGES.
  * Returns 1 if there was no error, if not, returns 0 if not enough data were available,
  * -1 if there was an error updating the appctx state st0 accordingly.
  */
@@ -1835,6 +1842,7 @@ static inline int peer_recv_msg(struct appctx *appctx, char *msg_head, size_t ms
 {
        int reql;
        struct stream_interface *si = appctx->owner;
+       char *cur;
 
        reql = co_getblk(si_oc(si), msg_head, 2 * sizeof(char), *totl);
        if (reql <= 0) /* closed or EOL not found */
@@ -1845,46 +1853,32 @@ static inline int peer_recv_msg(struct appctx *appctx, char *msg_head, size_t ms
        if (!(msg_head[1] & PEER_MSG_STKT_BIT_MASK))
                return 1;
 
+       /* This is a stick-table message, let's go on */
+
        /* Read and Decode message length */
-       reql = co_getblk(si_oc(si), &msg_head[2], sizeof(char), *totl);
+       msg_head    += *totl;
+       msg_head_sz -= *totl;
+       reql = co_data(si_oc(si)) - *totl;
+       if (reql > msg_head_sz)
+               reql = msg_head_sz;
+
+       reql = co_getblk(si_oc(si), msg_head, reql, *totl);
        if (reql <= 0) /* closed */
                goto incomplete;
 
-       *totl += reql;
-
-       if ((unsigned int)msg_head[2] < PEER_ENC_2BYTES_MIN) {
-               *msg_len = msg_head[2];
-       }
-       else {
-               int i;
-               char *cur;
-               char *end;
-
-               for (i = 3 ; i < msg_head_sz ; i++) {
-                       reql = co_getblk(si_oc(si), &msg_head[i], sizeof(char), *totl);
-                       if (reql <= 0) /* closed */
-                               goto incomplete;
-
-                       *totl += reql;
-
-                       if (!(msg_head[i] & PEER_MSG_STKT_BIT_MASK))
-                               break;
-               }
+       cur = msg_head;
+       *msg_len = intdecode(&cur, cur + reql);
+       if (!cur) {
+               /* the number is truncated, did we read enough ? */
+               if (reql < msg_head_sz)
+                       goto incomplete;
 
-               if (i == msg_head_sz) {
-                       /* malformed message */
-                       appctx->st0 = PEER_SESS_ST_ERRPROTO;
-                       return -1;
-               }
-               end = msg_head + msg_head_sz;
-               cur = &msg_head[2];
-               *msg_len = intdecode(&cur, end);
-               if (!cur) {
-                       /* malformed message */
-                       appctx->st0 = PEER_SESS_ST_ERRPROTO;
-                       return -1;
-               }
+               /* malformed message */
+               TRACE_PROTO("malformed message: too large length encoding", PEERS_EV_UPDTMSG);
+               appctx->st0 = PEER_SESS_ST_ERRPROTO;
+               return -1;
        }
+       *totl += cur - msg_head;
 
        /* Read message content */
        if (*msg_len) {
@@ -2499,7 +2493,7 @@ switchstate:
                                uint32_t msg_len = 0;
                                char *msg_cur = trash.area;
                                char *msg_end = trash.area;
-                               unsigned char msg_head[7];
+                               unsigned char msg_head[7]; // 2 + 5 for varint32
                                int totl = 0;
 
                                prev_state = appctx->st0;