MEDIUM: htx: Harden HTX API to avoid invalid uses when EOM was reached

With recent changes, the info about the end of the message is now carried by
a block only. In addition, it is really important to never mix HTTP blocks
with tunneled data because we must be able at any time to test the tail
block to know if the EOM flag is set or not. To avoid any invalid uses, the
API was reinforced. BUG_ONs were added to trigger errors when such uses are
detected.

Note it should not be a huge change because mixing http data and tunneled
data was already not supported.

diff --git a/include/haproxy/htx.h b/include/haproxy/htx.h
index 3550ee2c12d4c0fb6ccb820ed2898e827bb96803..c22c3dbf66594f2c2761d6d0f0ad5d5d03dec200 100644 (file)
--- a/include/haproxy/htx.h
+++ b/include/haproxy/htx.h
@@ -33,6 +33,7 @@
 extern struct htx htx_empty;
 
 struct htx_blk *htx_defrag(struct htx *htx, struct htx_blk *blk, uint32_t info);
+struct htx_blk *__htx_add_blk(struct htx *htx, enum htx_blk_type type, uint32_t blksz);
 struct htx_blk *htx_add_blk(struct htx *htx, enum htx_blk_type type, uint32_t blksz);
 struct htx_blk *htx_remove_blk(struct htx *htx, struct htx_blk *blk);
 struct htx_ret htx_find_offset(struct htx *htx, uint32_t offset);
@@ -262,6 +263,19 @@ static inline enum htx_blk_type htx_get_tail_type(const struct htx *htx)
        return (blk ? htx_get_blk_type(blk) : HTX_BLK_UNUSED);
 }
 
+/* Return true if the HTX message is not empty and the HTX_BLK_FL_EOM flag is set
+ * on the tail block, which means the message is complete. Otherwise, false is
+ * returned.
+ */
+static inline int htx_has_eom(const struct htx *htx)
+{
+       struct htx_blk *blk = htx_get_tail_blk(htx);
+
+       /* htx_remove_blk() must take care to never leave unused blocks on head and tail of the message */
+       BUG_ON_HOT(blk && (blk->flags & HTX_BLK_FL_EOM) && htx_get_blk_type(blk) == HTX_BLK_UNUSED);
+       return (blk != NULL && !!(blk->flags & HTX_BLK_FL_EOM));
+}
+
 /* Returns the position of block immediately before the one pointed by <pos>. If
  * the message is empty or if <pos> is the position of the head, -1 returned.
  */
@@ -517,16 +531,22 @@ static inline struct htx_sl *htx_add_stline(struct htx *htx, enum htx_blk_type t
 
 /* Adds an HTX block of type HDR in <htx>. It returns the new block on
  * success. Otherwise, it returns NULL. The header name is always lower cased.
+ *
+ * A header may be added in a message already ended. This function is
+ * responsible to move the header before any EOH block. So before any block that
+ * can carry the EOM flag. The unsafe version can be used to add the block here.
  */
 static inline struct htx_blk *htx_add_header(struct htx *htx, const struct ist name,
                                             const struct ist value)
 {
        struct htx_blk *blk, *prevblk;
+       int eom;
 
        if (name.len > HTX_HDR_NAME_MAX_LEN || value.len > HTX_HDR_VALUE_MAX_LEN)
                return NULL;
 
-       blk = htx_add_blk(htx, HTX_BLK_HDR, name.len + value.len);
+       eom = htx_has_eom(htx);
+       blk = __htx_add_blk(htx, HTX_BLK_HDR, name.len + value.len);
        if (!blk)
                return NULL;
 
@@ -545,21 +565,32 @@ static inline struct htx_blk *htx_add_header(struct htx *htx, const struct ist n
                blk = prevblk;
        }
 
+       /* The new block was moved before any block of a greater type, and the
+        * EOM flag can only be carried by such a block. So a message already
+        * ended must still be ended.
+        */
+       BUG_ON_HOT(eom && !htx_has_eom(htx));
        return blk;
 }
 
 /* Adds an HTX block of type TLR in <htx>. It returns the new block on
  * success. Otherwise, it returns NULL. The trailer name is always lower cased.
+ *
+ * A trailer may be added in a message already ended. This function is
+ * responsible to move the trailer before any EOT block. The EOM, if set must be
+ * carried by a EOT block. The unsafe version can be used to add the block here.
  */
 static inline struct htx_blk *htx_add_trailer(struct htx *htx, const struct ist name,
                                              const struct ist value)
 {
        struct htx_blk *blk, *prevblk;
+       int eom;
 
        if (name.len > HTX_HDR_NAME_MAX_LEN || value.len > HTX_HDR_VALUE_MAX_LEN)
                return NULL;
 
-       blk = htx_add_blk(htx, HTX_BLK_TLR, name.len + value.len);
+       eom = htx_has_eom(htx);
+       blk = __htx_add_blk(htx, HTX_BLK_TLR, name.len + value.len);
        if (!blk)
                return NULL;
 
@@ -578,6 +609,11 @@ static inline struct htx_blk *htx_add_trailer(struct htx *htx, const struct ist
                blk = prevblk;
        }
 
+       /* The new block was moved before any block of a greater type, and the
+        * EOM flag can only be carried by such a block. So a message already
+        * ended must still be ended.
+        */
+       BUG_ON_HOT(eom && !htx_has_eom(htx));
        return blk;
 }
 
@@ -821,18 +857,6 @@ static inline int htx_is_empty_noerr(const struct htx *htx)
        return (htx_is_empty(htx) && !(htx->flags & (HTX_FL_PARSING_ERROR|HTX_FL_PROCESSING_ERROR)));
 }
 
-/* Return true if the htx message is not empty and HTX_FL_HAS_EOM flag is set on
- * the tail block. Otherwise, false is returned.
- */
-static inline int htx_has_eom(const struct htx *htx)
-{
-       struct htx_blk *blk = htx_get_tail_blk(htx);
-
-       /* htx_remove_blk() must take care to never leave unused blocks on head and tail of the message */
-       BUG_ON_HOT(blk && (blk->flags & HTX_BLK_FL_EOM) && htx_get_blk_type(blk) == HTX_BLK_UNUSED);
-       return (blk != NULL && !!(blk->flags & HTX_BLK_FL_EOM));
-}
-
 /* Returns 1 if more data are expected for the message <htx>. Otherwise it
  * returns 0. Note that it is illegal to call this with htx == NULL. This
  * function relies on htx_has_eom() function. It means more data are always

diff --git a/src/htx.c b/src/htx.c
index aa473d5554e6e009ea87d2c6e36b14eb9977dd32..166fda0593c8cc7930d1ba74a2caab5daee57639 100644 (file)
--- a/src/htx.c
+++ b/src/htx.c
@@ -18,6 +18,9 @@
 
 struct htx htx_empty = { .size = 0, .data = 0, .head  = -1, .tail = -1, .first = -1 };
 
+
+struct htx_blk *__htx_add_data_atonce(struct htx *htx, struct ist data);
+
 /* tests show that 63% of these calls are for 64-bit chunks, so better avoid calling
  * memcpy() for that!
  */
@@ -74,17 +77,17 @@ struct htx_blk *htx_defrag(struct htx *htx, struct htx_blk *blk, uint32_t blkinf
                switch (type) {
                        case HTX_BLK_DATA:
                                if (blk != oldblk) {
-                                       newblk = htx_add_data_atonce(tmp, htx_get_blk_value(htx, oldblk));
+                                       newblk = __htx_add_data_atonce(tmp, htx_get_blk_value(htx, oldblk));
                                        break;
                                }
                                __fallthrough;
                        default:
                                if (blk == oldblk && blkinfo) {
-                                       newblk = htx_add_blk(tmp, type, __htx_blkinfo_size(blkinfo));
+                                       newblk = __htx_add_blk(tmp, type, __htx_blkinfo_size(blkinfo));
                                        newblk->info = blkinfo;
                                }
                                else {
-                                       newblk = htx_add_blk(tmp, type, blksz);
+                                       newblk = __htx_add_blk(tmp, type, blksz);
                                        newblk->info = oldblk->info;
                                }
                                htx_memcpy(htx_get_blk_ptr(tmp, newblk), htx_get_blk_ptr(htx, oldblk), blksz);
@@ -350,8 +353,13 @@ static int htx_prepare_blk_expansion(struct htx *htx, struct htx_blk *blk, int32
 
 /* Adds a new block of type <type> in the HTX message <htx>. Its content size is
  * passed but it is the caller responsibility to do the copy.
+ *
+ * This is the unsafe version. It does not check the message was not already
+ * ended. It must only be used by the HTX API itself, to temporarily add a block
+ * after the one carrying the EOM flag, before moving it at the right place. Any
+ * other caller must use htx_add_blk() instead.
  */
-struct htx_blk *htx_add_blk(struct htx *htx, enum htx_blk_type type, uint32_t blksz)
+struct htx_blk *__htx_add_blk(struct htx *htx, enum htx_blk_type type, uint32_t blksz)
 {
        struct htx_blk *blk;
 
@@ -370,6 +378,18 @@ struct htx_blk *htx_add_blk(struct htx *htx, enum htx_blk_type type, uint32_t bl
        return blk;
 }
 
+/* Adds a new block of type <type> in the HTX message <htx>. Its content size is
+ * passed but it is the caller responsibility to do the copy.
+ *
+ * Nothing must be added in a message already ended. Otherwise HTTP data and
+ * tunneled data could be mixed. The message must be fully consumed first.
+ */
+struct htx_blk *htx_add_blk(struct htx *htx, enum htx_blk_type type, uint32_t blksz)
+{
+       BUG_ON(htx_has_eom(htx));
+       return __htx_add_blk(htx, type, blksz);
+}
+
 /* Removes the block <blk> from the HTX message <htx>. The function returns the
  * block following <blk> or NULL if <blk> is the last block or the last inserted
  * one.
@@ -565,8 +585,13 @@ struct htx_ret htx_drain(struct htx *htx, uint32_t count)
  * returned. Otherwise, on success, the updated block (or the new one) is
  * returned. Due to its nature this function can be expensive and should be
  * avoided whenever possible.
+ *
+ * This is the unsafe version. It does not check the message was not already
+ * ended. It must only be used by the HTX API itself, to temporarily add a block
+ * after the one carrying the EOM flag, before moving it at the right place. Any
+ * other caller must use htx_add_data_atonce() instead.
  */
-struct htx_blk *htx_add_data_atonce(struct htx *htx, struct ist data)
+struct htx_blk *__htx_add_data_atonce(struct htx *htx, struct ist data)
 {
        struct htx_blk *blk, *tailblk;
        void *ptr;
@@ -632,7 +657,7 @@ struct htx_blk *htx_add_data_atonce(struct htx *htx, struct ist data)
        goto end;
 
   add_new_block:
-       blk = htx_add_blk(htx, HTX_BLK_DATA, data.len);
+       blk = __htx_add_blk(htx, HTX_BLK_DATA, data.len);
        if (!blk)
                return NULL;
        blk->info += data.len;
@@ -647,6 +672,21 @@ struct htx_blk *htx_add_data_atonce(struct htx *htx, struct ist data)
        return blk;
 }
 
+/* Tries to append data to the last inserted block, if the type matches and if
+ * there is enough space to take it all. If the space wraps, the buffer is
+ * defragmented and a new block is inserted. If an error occurred, NULL is
+ * returned. Otherwise, on success, the updated block (or the new one) is
+ * returned.
+ *
+ * No data must be added in a message already ended. Otherwise HTTP data and
+ * tunneled data could be mixed. The message must be fully consumed first.
+ */
+struct htx_blk *htx_add_data_atonce(struct htx *htx, struct ist data)
+{
+       BUG_ON(htx_has_eom(htx));
+       return __htx_add_data_atonce(htx, data);
+}
+
 /* Replaces a value part of a block by a new one. The new part can be smaller or
  * larger than the old one. This function works for any kind of block with
  * attached data. It returns the new block on success, otherwise it returns
@@ -785,6 +825,7 @@ size_t htx_xfer(struct htx *dst, struct htx *src, size_t count, unsigned int fla
        uint32_t max, last_dstblk_sz;
        int dst_full = 0;
 
+       BUG_ON(htx_has_eom(dst));
 
        last_dstblk = NULL;
        last_dstblk_sz = 0;
@@ -1050,6 +1091,8 @@ struct htx_ret htx_reserve_max_data(struct htx *htx)
        int32_t len = htx_free_data_space(htx);
        uint32_t flags = 0;
 
+       BUG_ON(htx_has_eom(htx));
+
        if (!len)
                return (struct htx_ret){.ret = 0, .blk = NULL};
 
@@ -1125,6 +1168,8 @@ size_t htx_add_data(struct htx *htx, const struct ist data)
        int32_t len = data.len;
        uint32_t flags = 0;
 
+       BUG_ON(htx_has_eom(htx));
+
        /* Not enough space to store data */
        if (len > htx_free_data_space(htx))
                len = htx_free_data_space(htx);
@@ -1209,8 +1254,14 @@ size_t htx_add_data(struct htx *htx, const struct ist data)
 struct htx_blk *htx_add_last_data(struct htx *htx, struct ist data)
 {
        struct htx_blk *blk, *pblk;
+       int eom = htx_has_eom(htx);
 
-       blk = htx_add_data_atonce(htx, data);
+       /* Data may be added in a message already ended. In this case, the block
+        * is first appended, after the block carrying the EOM flag, and then
+        * moved after the last DATA block. So the unsafe version is used here and
+        * the EOM flag is transferred below.
+        */
+       blk = __htx_add_data_atonce(htx, data);
        if (!blk)
                return NULL;
 
@@ -1231,6 +1282,10 @@ struct htx_blk *htx_add_last_data(struct htx *htx, struct ist data)
                blk = pblk;
        }
 
+       /* The EOM flag was transferred on the new DATA block, or moved with the
+        * blocks placed after it. So a message already ended must still be ended.
+        */
+       BUG_ON_HOT(eom && !htx_has_eom(htx));
        return blk;
 }
 
@@ -1282,6 +1337,8 @@ int htx_append_msg(struct htx *dst, const struct htx *src)
        enum htx_blk_type type;
        uint32_t blksz, offset = dst->data;
 
+       BUG_ON(htx_has_eom(dst));
+
        for (blk = htx_get_head_blk(src); blk; blk = htx_get_next_blk(src, blk)) {
                type = htx_get_blk_type(blk);
                blksz = htx_get_blksz(blk);