Add warning about a potential pitfall with WPACKET_allocate_bytes()

If the underlying BUF_MEM gets realloc'd then the pointer returned could
become invalid. Therefore we should always ensure that the allocated
memory is filled in prior to any more WPACKET_* calls.

Reviewed-by: Rich Salz <rsalz@openssl.org>

diff --git a/ssl/packet.c b/ssl/packet.c
index 6199469969abb5e828adfca68f3b4694c311ef55..0e8e8764dd8dd0193d7666b4f13d43acbe368e64 100644 (file)
--- a/ssl/packet.c
+++ b/ssl/packet.c
@@ -224,6 +224,7 @@ int WPACKET_start_sub_packet_len__(WPACKET *pkt, size_t lenbytes)
 
     if (!WPACKET_allocate_bytes(pkt, lenbytes, &lenchars))
         return 0;
+    /* Convert to an offset in case the underlying BUF_MEM gets realloc'd */
     sub->packet_len = lenchars - (unsigned char *)pkt->buf->data;
 
     return 1;

diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h
index c51d8922a8f06ec0a3d45059a6b25a2b0d3ac069..44a8f82c7c6076dbf3be2baf38ed244a659fd913 100644 (file)
--- a/ssl/packet_locl.h
+++ b/ssl/packet_locl.h
@@ -671,6 +671,9 @@ int WPACKET_start_sub_packet(WPACKET *pkt);
  * Allocate bytes in the WPACKET for the output. This reserves the bytes
  * and counts them as "written", but doesn't actually do the writing. A pointer
  * to the allocated bytes is stored in |*allocbytes|.
+ * WARNING: the allocated bytes must be filled in immediately, without further
+ * WPACKET_* calls. If not then the underlying buffer may be realloc'd and
+ * change its location.
  */
 int WPACKET_allocate_bytes(WPACKET *pkt, size_t bytes,
                            unsigned char **allocbytes);
@@ -715,7 +718,7 @@ int WPACKET_put_bytes__(WPACKET *pkt, unsigned int val, size_t bytes);
 #define WPACKET_put_bytes_u16(pkt, val) \
     WPACKET_put_bytes__((pkt), (val), 2)
 #define WPACKET_put_bytes_u24(pkt, val) \
-    WPACKET_put_bytes__((pkt), (val)), 3)
+    WPACKET_put_bytes__((pkt), (val), 3)
 #define WPACKET_put_bytes_u32(pkt, val) \
     WPACKET_sub_allocate_bytes__((pkt), (val), 4)