From: John Glotzer Date: Mon, 4 Aug 2014 19:39:23 +0000 (+0000) Subject: bgpd: memmove needed in community_del_val X-Git-Tag: frr-2.0-rc1~1572^2~2 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=4c005e3f65a1f5b4592b1ebbac392cbb1a710998;p=mirror%2Ffrr.git 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 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 --- diff --git a/bgpd/bgp_community.c b/bgpd/bgp_community.c index fc1bef88b7..1bd2dd84e4 100644 --- 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--;