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.
(cherry picked from commit 1dfd4f106f15bc4e6e992f8babbc863c12975b5a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
diff --git a/src/peers.c b/src/peers.c
index 646fe46..afe770c 100644
--- a/src/peers.c
+++ b/src/peers.c
@@ -1712,7 +1712,14 @@
}
/*
- * 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.
*/
@@ -1721,6 +1728,7 @@
{
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 */
@@ -1731,46 +1739,31 @@
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 */
+ appctx->st0 = PEER_SESS_ST_ERRPROTO;
+ return -1;
}
+ *totl += cur - msg_head;
/* Read message content */
if (*msg_len) {
@@ -2385,7 +2378,7 @@
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;