Merge pull request #1133 from opensourcerouting/shift-sign

*: fix be32 reading / 24-bit left shift

diff --git a/bgpd/bgp_attr_evpn.c b/bgpd/bgp_attr_evpn.c
index 6ead059261e635da03fcb3dbbcadbeac1fe8e154..300c9ddb506a4f471a788ba351dfa4af33474986 100644 (file)
--- a/bgpd/bgp_attr_evpn.c
+++ b/bgpd/bgp_attr_evpn.c
@@ -144,11 +144,8 @@ u_int32_t bgp_attr_mac_mobility_seqnum(struct attr *attr, u_char *sticky)
                        *sticky = 0;
 
                pnt++;
-               seq_num = (*pnt++ << 24);
-               seq_num |= (*pnt++ << 16);
-               seq_num |= (*pnt++ << 8);
-               seq_num |= (*pnt++);
-
+               pnt = ptr_get_be32(pnt, &seq_num);
+               (void)pnt; /* consume value */
                return seq_num;
        }
 

diff --git a/bgpd/bgp_clist.c b/bgpd/bgp_clist.c
index b62a0a540c3ef0412476103a908e390cf79b4bf3..f3bae9535cc92b12200a6cddec2c3f2ac67f0239 100644 (file)
--- a/bgpd/bgp_clist.c
+++ b/bgpd/bgp_clist.c
@@ -25,6 +25,7 @@
 #include "memory.h"
 #include "queue.h"
 #include "filter.h"
+#include "stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_community.h"
@@ -465,20 +466,10 @@ static char *lcommunity_str_get(struct lcommunity *lcom, int i)
        str = pnt = XMALLOC(MTYPE_LCOMMUNITY_STR, 48);
 
        ptr = (u_char *)lcomval.val;
-       globaladmin = (*ptr++ << 24);
-       globaladmin |= (*ptr++ << 16);
-       globaladmin |= (*ptr++ << 8);
-       globaladmin |= (*ptr++);
-
-       localdata1 = (*ptr++ << 24);
-       localdata1 |= (*ptr++ << 16);
-       localdata1 |= (*ptr++ << 8);
-       localdata1 |= (*ptr++);
-
-       localdata2 = (*ptr++ << 24);
-       localdata2 |= (*ptr++ << 16);
-       localdata2 |= (*ptr++ << 8);
-       localdata2 |= (*ptr++);
+       ptr = ptr_get_be32(ptr, &globaladmin);
+       ptr = ptr_get_be32(ptr, &localdata1);
+       ptr = ptr_get_be32(ptr, &localdata2);
+       (void)ptr; /* consume value */
 
        sprintf(pnt, "%u:%u:%u", globaladmin, localdata1, localdata2);
        pnt += strlen(pnt);

diff --git a/bgpd/bgp_ecommunity.c b/bgpd/bgp_ecommunity.c
index ec52422d4cf1ae4f0db7c014914fc795561b67aa..897b1c7508c275e371431f502c159d38493549fc 100644 (file)
--- a/bgpd/bgp_ecommunity.c
+++ b/bgpd/bgp_ecommunity.c
@@ -27,6 +27,7 @@
 #include "queue.h"
 #include "filter.h"
 #include "jhash.h"
+#include "stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_ecommunity.h"
@@ -583,10 +584,7 @@ static int ecommunity_rt_soo_str(char *buf, u_int8_t *pnt, int type,
 
        /* Put string into buffer.  */
        if (type == ECOMMUNITY_ENCODE_AS4) {
-               eas.as = (*pnt++ << 24);
-               eas.as |= (*pnt++ << 16);
-               eas.as |= (*pnt++ << 8);
-               eas.as |= (*pnt++);
+               pnt = ptr_get_be32(pnt, &eas.as);
                eas.val = (*pnt++ << 8);
                eas.val |= (*pnt++);
 
@@ -594,11 +592,7 @@ static int ecommunity_rt_soo_str(char *buf, u_int8_t *pnt, int type,
        } else if (type == ECOMMUNITY_ENCODE_AS) {
                eas.as = (*pnt++ << 8);
                eas.as |= (*pnt++);
-
-               eas.val = (*pnt++ << 24);
-               eas.val |= (*pnt++ << 16);
-               eas.val |= (*pnt++ << 8);
-               eas.val |= (*pnt++);
+               pnt = ptr_get_be32(pnt, &eas.val);
 
                len = sprintf(buf, "%s%u:%u", prefix, eas.as, eas.val);
        } else if (type == ECOMMUNITY_ENCODE_IP) {
@@ -610,6 +604,7 @@ static int ecommunity_rt_soo_str(char *buf, u_int8_t *pnt, int type,
                len = sprintf(buf, "%s%s:%u", prefix, inet_ntoa(eip.ip),
                              eip.val);
        }
+       (void)pnt; /* consume value */
 
        return len;
 }

diff --git a/bgpd/bgp_encap_tlv.c b/bgpd/bgp_encap_tlv.c
index 5c0cc40f16c2e35826e69c381f0a91efd57e58f6..4457501613a1b25dde212471c3293a35cebac5bb 100644 (file)
--- a/bgpd/bgp_encap_tlv.c
+++ b/bgpd/bgp_encap_tlv.c
@@ -22,6 +22,7 @@
 #include "memory.h"
 #include "prefix.h"
 #include "filter.h"
+#include "stream.h"
 
 #include "bgpd.h"
 #include "bgp_attr.h"
@@ -470,8 +471,7 @@ static int subtlv_decode_encap_l2tpv3_over_ip(
                return -1;
        }
 
-       st->sessionid = (subtlv->value[0] << 24) | (subtlv->value[1] << 16)
-                       | (subtlv->value[2] << 8) | subtlv->value[3];
+       ptr_get_be32(subtlv->value, &st->sessionid);
        st->cookie_length = subtlv->length - 4;
        if (st->cookie_length > sizeof(st->cookie)) {
                zlog_debug("%s, subtlv length %d is greater than %d", __func__,
@@ -491,8 +491,7 @@ static int subtlv_decode_encap_gre(struct bgp_attr_encap_subtlv *subtlv,
                           subtlv->length);
                return -1;
        }
-       st->gre_key = (subtlv->value[0] << 24) | (subtlv->value[1] << 16)
-                     | (subtlv->value[2] << 8) | subtlv->value[3];
+       ptr_get_be32(subtlv->value, &st->gre_key);
        return 0;
 }
 
@@ -545,8 +544,7 @@ static int subtlv_decode_color(struct bgp_attr_encap_subtlv *subtlv,
                           __func__);
                return -1;
        }
-       st->color = (subtlv->value[4] << 24) | (subtlv->value[5] << 16)
-                   | (subtlv->value[6] << 8) | subtlv->value[7];
+       ptr_get_be32(subtlv->value + 4, &st->color);
        return 0;
 }
 
@@ -580,16 +578,13 @@ subtlv_decode_remote_endpoint(struct bgp_attr_encap_subtlv *subtlv,
        }
        if (subtlv->length == 8) {
                st->family = AF_INET;
-               st->ip_address.v4.s_addr =
-                       ((subtlv->value[0] << 24) | (subtlv->value[1] << 16)
-                        | (subtlv->value[2] << 8) | subtlv->value[3]);
+               memcpy(&st->ip_address.v4.s_addr, subtlv->value, 4);
        } else {
                st->family = AF_INET6;
                memcpy(&(st->ip_address.v6.s6_addr), subtlv->value, 16);
        }
        i = subtlv->length - 4;
-       st->as4 = ((subtlv->value[i] << 24) | (subtlv->value[i + 1] << 16)
-                  | (subtlv->value[i + 2] << 8) | subtlv->value[i + 3]);
+       ptr_get_be32(subtlv->value + i, &st->as4);
        return 0;
 }
 

diff --git a/bgpd/bgp_evpn_vty.c b/bgpd/bgp_evpn_vty.c
index 2410f30ddfa65d62e59e1cdeb10e82e0b0858c22..17511827b1b8fcffde582f53b2d7a3a1f48b1fa8 100644 (file)
--- a/bgpd/bgp_evpn_vty.c
+++ b/bgpd/bgp_evpn_vty.c
@@ -22,6 +22,7 @@
 #include "command.h"
 #include "prefix.h"
 #include "lib/json.h"
+#include "stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_table.h"
@@ -87,11 +88,7 @@ static void display_import_rt(struct vty *vty, struct irt_node *irt,
        case ECOMMUNITY_ENCODE_AS:
                eas.as = (*pnt++ << 8);
                eas.as |= (*pnt++);
-
-               eas.val = (*pnt++ << 24);
-               eas.val |= (*pnt++ << 16);
-               eas.val |= (*pnt++ << 8);
-               eas.val |= (*pnt++);
+               pnt = ptr_get_be32(pnt, &eas.val);
 
                snprintf(rt_buf, RT_ADDRSTRLEN, "%u:%u", eas.as, eas.val);
 
@@ -119,11 +116,7 @@ static void display_import_rt(struct vty *vty, struct irt_node *irt,
                break;
 
        case ECOMMUNITY_ENCODE_AS4:
-               eas.as = (*pnt++ << 24);
-               eas.as |= (*pnt++ << 16);
-               eas.as |= (*pnt++ << 8);
-               eas.as |= (*pnt++);
-
+               pnt = ptr_get_be32(pnt, &eas.val);
                eas.val = (*pnt++ << 8);
                eas.val |= (*pnt++);
 

diff --git a/bgpd/bgp_lcommunity.c b/bgpd/bgp_lcommunity.c
index d75f33f58b827cf609ebc2991cf6b6100acf94f7..54e9fd889457757d563c5772a9d56dd6bc66d15b 100644 (file)
--- a/bgpd/bgp_lcommunity.c
+++ b/bgpd/bgp_lcommunity.c
@@ -26,6 +26,7 @@
 #include "command.h"
 #include "filter.h"
 #include "jhash.h"
+#include "stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_lcommunity.h"
@@ -431,20 +432,10 @@ char *lcommunity_lcom2str(struct lcommunity *lcom, int format)
 
                pnt = lcom->val + (i * LCOMMUNITY_SIZE);
 
-               globaladmin = (*pnt++ << 24);
-               globaladmin |= (*pnt++ << 16);
-               globaladmin |= (*pnt++ << 8);
-               globaladmin |= (*pnt++);
-
-               localdata1 = (*pnt++ << 24);
-               localdata1 |= (*pnt++ << 16);
-               localdata1 |= (*pnt++ << 8);
-               localdata1 |= (*pnt++);
-
-               localdata2 = (*pnt++ << 24);
-               localdata2 |= (*pnt++ << 16);
-               localdata2 |= (*pnt++ << 8);
-               localdata2 |= (*pnt++);
+               pnt = ptr_get_be32(pnt, &globaladmin);
+               pnt = ptr_get_be32(pnt, &localdata1);
+               pnt = ptr_get_be32(pnt, &localdata2);
+               (void)pnt; /* consume value */
 
                len = sprintf(str_buf + str_pnt, "%u:%u:%u", globaladmin,
                              localdata1, localdata2);

diff --git a/bgpd/bgp_rd.c b/bgpd/bgp_rd.c
index a15828bd36a9402194ff82d72ba24b7a0dc97c97..2b676e052b3480b31ac72c2938ff0e69df317918 100644 (file)
--- a/bgpd/bgp_rd.c
+++ b/bgpd/bgp_rd.c
@@ -63,21 +63,13 @@ void decode_rd_as(u_char *pnt, struct rd_as *rd_as)
 {
        rd_as->as = (u_int16_t)*pnt++ << 8;
        rd_as->as |= (u_int16_t)*pnt++;
-
-       rd_as->val = ((u_int32_t)*pnt++ << 24);
-       rd_as->val |= ((u_int32_t)*pnt++ << 16);
-       rd_as->val |= ((u_int32_t)*pnt++ << 8);
-       rd_as->val |= (u_int32_t)*pnt;
+       ptr_get_be32(pnt, &rd_as->val);
 }
 
 /* type == RD_TYPE_AS4 */
 void decode_rd_as4(u_char *pnt, struct rd_as *rd_as)
 {
-       rd_as->as = (u_int32_t)*pnt++ << 24;
-       rd_as->as |= (u_int32_t)*pnt++ << 16;
-       rd_as->as |= (u_int32_t)*pnt++ << 8;
-       rd_as->as |= (u_int32_t)*pnt++;
-
+       pnt = ptr_get_be32(pnt, &rd_as->as);
        rd_as->val = ((u_int16_t)*pnt++ << 8);
        rd_as->val |= (u_int16_t)*pnt;
 }

diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c
index 3f427c39031ee336ec05c1b44fda998dadc3c392..d963a759ace65927c6d9cc2e8fb04cf0335a46ef 100644 (file)
--- a/bgpd/rfapi/rfapi_import.c
+++ b/bgpd/rfapi/rfapi_import.c
@@ -33,6 +33,7 @@
 #include "lib/log.h"
 #include "lib/skiplist.h"
 #include "lib/thread.h"
+#include "lib/stream.h"
 
 #include "bgpd/bgpd.h"
 #include "bgpd/bgp_ecommunity.h"
@@ -1079,10 +1080,7 @@ int rfapiEcommunityGetEthernetTag(struct ecommunity *ecom, uint16_t *tag_id)
 
                        if (*p++ == ECOMMUNITY_ROUTE_TARGET) {
                                if (encode == ECOMMUNITY_ENCODE_AS4) {
-                                       as = (*p++ << 24);
-                                       as |= (*p++ << 16);
-                                       as |= (*p++ << 8);
-                                       as |= (*p++);
+                                       p = ptr_get_be32(p, &as);
                                } else if (encode == ECOMMUNITY_ENCODE_AS) {
                                        as = (*p++ << 8);
                                        as |= (*p++);

diff --git a/lib/skiplist.c b/lib/skiplist.c
index 7acc78f563729e7f32aef1cd4074cedab6376b4e..a546bb44c0250ead36af58b7a104b6745d1ce126 100644 (file)
--- a/lib/skiplist.c
+++ b/lib/skiplist.c
@@ -593,8 +593,8 @@ static void *scramble(int i)
 {
        uintptr_t result;
 
-       result = (i & 0xff) << 24;
-       result |= (i >> 8);
+       result = (unsigned)(i & 0xff) << 24;
+       result |= (unsigned)i >> 8;
 
        return (void *)result;
 }

diff --git a/lib/stream.c b/lib/stream.c
index 577fa257df53df53b01b6ae83be213986afcfbee..f88689f677c4842f6226c3657c8eb170bb63a21e 100644 (file)
--- a/lib/stream.c
+++ b/lib/stream.c
@@ -394,7 +394,7 @@ u_int32_t stream_getl_from(struct stream *s, size_t from)
                return 0;
        }
 
-       l = s->data[from++] << 24;
+       l = (unsigned)(s->data[from++]) << 24;
        l |= s->data[from++] << 16;
        l |= s->data[from++] << 8;
        l |= s->data[from];
@@ -426,7 +426,7 @@ u_int32_t stream_getl(struct stream *s)
                return 0;
        }
 
-       l = s->data[s->getp++] << 24;
+       l = (unsigned)(s->data[s->getp++]) << 24;
        l |= s->data[s->getp++] << 16;
        l |= s->data[s->getp++] << 8;
        l |= s->data[s->getp++];

diff --git a/lib/stream.h b/lib/stream.h
index 33dd64c4a399eb89ca89bb1f8f9a337b4406565e..7dcdca69f6db8aa0f8dfa03c7fa110937730854d 100644 (file)
--- a/lib/stream.h
+++ b/lib/stream.h
@@ -239,4 +239,22 @@ extern struct stream *stream_fifo_head(struct stream_fifo *fifo);
 extern void stream_fifo_clean(struct stream_fifo *fifo);
 extern void stream_fifo_free(struct stream_fifo *fifo);
 
+/* This is here because "<< 24" is particularly problematic in C.
+ * This is because the left operand of << is integer-promoted, which means
+ * an uint8_t gets converted into a *signed* int.  Shifting into the sign
+ * bit of a signed int is theoretically undefined behaviour, so - the left
+ * operand needs to be cast to unsigned.
+ *
+ * This is not a problem for 16- or 8-bit values (they don't reach the sign
+ * bit), for 64-bit values (you need to cast them anyway), and neither for
+ * encoding (because it's downcasted.)
+ */
+static inline uint8_t *ptr_get_be32(uint8_t *ptr, uint32_t *out)
+{
+       uint32_t tmp;
+       memcpy(&tmp, ptr, sizeof(tmp));
+       *out = ntohl(tmp);
+       return ptr + 4;
+}
+
 #endif /* _ZEBRA_STREAM_H */

diff --git a/ospfd/ospf_opaque.h b/ospfd/ospf_opaque.h
index 2470cd2e2b2f1d7bb90cf8acb288c1c0a7c0d501..9dc1f92f4dc16ab0decc992dd94c3fdb1405342d 100644 (file)
--- a/ospfd/ospf_opaque.h
+++ b/ospfd/ospf_opaque.h
@@ -46,7 +46,8 @@
 #define GET_OPAQUE_ID(lsid) ((u_int32_t)(lsid)&LSID_OPAQUE_ID_MASK)
 
 #define SET_OPAQUE_LSID(type, id)                                              \
-       ((((type) << 24) & LSID_OPAQUE_TYPE_MASK) | ((id)&LSID_OPAQUE_ID_MASK))
+       ((((unsigned)(type) << 24) & LSID_OPAQUE_TYPE_MASK)                    \
+        | ((id) & LSID_OPAQUE_ID_MASK))
 
 /*
  * Opaque LSA types will be assigned by IANA.