Fix MAX_PKT{4,6}_SZ to account for icmpEchoData padding (#887)

The bug was exposed by GCC v11 on Ubuntu Impish:

    Icmp4.cc:116:11: error: array subscript icmpEchoData[0] is partly
        outside array bounds of char[282] [-Werror=array-bounds]
        echo->opcode = (unsigned char) opcode;

The array the compiler is talking about is the pkt buffer. That buffer
size (i.e. MAX_PKT4_SZ) was calculated under the faulty assumption that
a compiler cannot add padding after icmphdr (when doing "icmp+1") and/or
between icmpEchoData data members. When compiler padded, the old
MAX_PKT4_SZ math stopped working.

Same for ICMPv6.

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index e783fe13c14a4e3bc0192bda41ba87193a155f95..e9c28fa048dd8c39539301b6d82d96cb0badd678 100644 (file)
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -415,6 +415,7 @@ Thank you!
     Sebastian Krahmer <krahmer@suse.com>
     Sebastien Wenske <sebastien@wenske.fr>
     Sergey Merzlikin <sm@smsoft.ru>
+    Sergio Durigan Junior <sergiodj@sergiodj.net>
     Sergio Rabellino <rabellino@di.unito.it>
     Shigechika Aikawa <shige@luck.imasy.or.jp>
     Silamael <Silamael@coronamundi.de>

diff --git a/src/icmp/Icmp.h b/src/icmp/Icmp.h
index dce63c21d1965f6524fbb63c9d541e652271d35e..ed187a0ef1823f0f741ea96a9a5933564bd1ecd4 100644 (file)
--- a/src/icmp/Icmp.h
+++ b/src/icmp/Icmp.h
@@ -16,8 +16,8 @@
 #define PINGER_PAYLOAD_SZ   8192
 
 #define MAX_PAYLOAD 256 // WAS: SQUIDHOSTNAMELEN
-#define MAX_PKT4_SZ (MAX_PAYLOAD + sizeof(struct timeval) + sizeof (char) + sizeof(struct icmphdr) + 1)
-#define MAX_PKT6_SZ (MAX_PAYLOAD + sizeof(struct timeval) + sizeof (char) + sizeof(struct icmp6_hdr) + 1)
+#define MAX_PKT4_SZ (sizeof(struct icmphdr) + sizeof(struct icmpEchoData) + 1)
+#define MAX_PKT6_SZ (sizeof(struct icmp6_hdr) + sizeof(struct icmpEchoData) + 1)
 
 #if USE_ICMP
 

diff --git a/src/icmp/Icmp4.cc b/src/icmp/Icmp4.cc
index 25ff8a6fdf3d75a425483312854705a94f009463..df7a51506a2a4ec9d244c05dd582098066ed6ee5 100644 (file)
--- a/src/icmp/Icmp4.cc
+++ b/src/icmp/Icmp4.cc
@@ -90,6 +90,8 @@ Icmp4::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     size_t icmp_pktsize = sizeof(struct icmphdr);
     struct addrinfo *S = NULL;
 
+    static_assert(sizeof(*icmp) + sizeof(*echo) <= sizeof(pkt), "our custom ICMPv4 Echo payload fits the packet buffer");
+
     memset(pkt, '\0', MAX_PKT4_SZ);
 
     icmp = (struct icmphdr *) (void *) pkt;
@@ -111,7 +113,7 @@ Icmp4::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     ++icmp_pkts_sent;
 
     // Construct ICMP packet data content
-    echo = (icmpEchoData *) (icmp + 1);
+    echo = reinterpret_cast<icmpEchoData *>(reinterpret_cast<char *>(pkt) + sizeof(*icmp));
     echo->opcode = (unsigned char) opcode;
     memcpy(&echo->tv, &current_time, sizeof(struct timeval));
 

diff --git a/src/icmp/Icmp6.cc b/src/icmp/Icmp6.cc
index c5fb004277d702819f9f3236ce4764392b57069d..3c176bee2055b74a8c055db38ebde02a89264991 100644 (file)
--- a/src/icmp/Icmp6.cc
+++ b/src/icmp/Icmp6.cc
@@ -124,6 +124,8 @@ Icmp6::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     struct addrinfo *S = NULL;
     size_t icmp6_pktsize = 0;
 
+    static_assert(sizeof(*icmp) + sizeof(*echo) <= sizeof(pkt), "our custom ICMPv6 Echo payload fits the packet buffer");
+
     memset(pkt, '\0', MAX_PKT6_SZ);
     icmp = (struct icmp6_hdr *)pkt;
 
@@ -146,7 +148,7 @@ Icmp6::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     icmp6_pktsize = sizeof(struct icmp6_hdr);
 
     // Fill Icmp6 ECHO data content
-    echo = (icmpEchoData *) (pkt + sizeof(icmp6_hdr));
+    echo = reinterpret_cast<icmpEchoData *>(reinterpret_cast<char *>(pkt) + sizeof(*icmp));
     echo->opcode = (unsigned char) opcode;
     memcpy(&echo->tv, &current_time, sizeof(struct timeval));