bgpd: memmove needed in community_del_val

In bgpd/bgp_community_del_val memcpy is used for potentially overlapping
regions which is *not* safe. It may "work" in some cases but is not
guaranteed to work in all cases. The case that I saw fail was on an
x86_64 architecture with the number of bytes being moved/copied equal to
8.

The way the code is written the uint32_t pointers will always differ by
1, which is equivalent to a memcpy/memmove of regions that are 4 bytes
away from one another. So the code failed while copying an 8 byte region
to an address that is 4 bytes lower i.e. overlapping regions.

Interestingly, the same architecture had no problems with a 12 byte
copy.

When the code failed the communities were [200,300,400] and a call was
made to delete the 200 community. The result of this was an array that
looked like [400,400] which was uniquified to [400]. Of course the
expected result should have been [300, 400].

One additional point - in our production environment memmove would not
*link* without including <string.h> but in an isolated quagga git repo
this #include does not seem to be required and I see memmove is used in
vtysh.c without this #include either.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>

diff --git a/bgpd/bgp_community.c b/bgpd/bgp_community.c
index fc1bef88b7bf63b2dc6196249d0a06fc4b868f19..1bd2dd84e44b292e2257ef512ca444ff3936c8b5 100644 (file)
--- a/bgpd/bgp_community.c
+++ b/bgpd/bgp_community.c
@@ -78,7 +78,7 @@ community_del_val (struct community *com, u_int32_t *val)
          c = com->size -i -1;
 
          if (c > 0)
-           memcpy (com->val + i, com->val + (i + 1), c * sizeof (*val));
+           memmove (com->val + i, com->val + (i + 1), c * sizeof (*val));
 
          com->size--;