[ovs-dev] [PATCH 3/4] userspace: Fix strncpy() errors

Russell Bryant rbryant at redhat.com
Fri Feb 20 18:22:16 UTC 2015


strncpy() has pretty annoying semantics.  One bit that is odd is that
it does not guarantee that the destination string will be null
terminated.  If no terminator is found in the first N bytes of the
source, it just fills the destination buffer and doesn't terminate it.

The changes here are to address cases where the destination could be
left without a terminator.  All of these cases were using a constant
for the 3rd argument.  These changes prefer using sizeof on the
destination where possible, even though it results in the same thing.
In cases where we know the destination has been zeroed out, the fix is
just subtracting 1 from the destination size.  In one case, explicitly
writing a terminating byte is required.

Signed-off-by: Russell Bryant <rbryant at redhat.com>
---
 lib/ovs-router.c             | 5 +++--
 lib/tnl-arp-cache.c          | 3 ++-
 lib/tnl-ports.c              | 2 +-
 ofproto/ofproto-dpif-xlate.c | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/ovs-router.c b/lib/ovs-router.c
index 8de2e2d..e5db5a7 100644
--- a/lib/ovs-router.c
+++ b/lib/ovs-router.c
@@ -72,7 +72,8 @@ ovs_router_lookup(ovs_be32 ip_dst, char output_bridge[], ovs_be32 *gw)
     if (cr) {
         struct ovs_router_entry *p = ovs_router_entry_cast(cr);
 
-        strncpy(output_bridge, p->output_bridge, IFNAMSIZ);
+        strncpy(output_bridge, p->output_bridge, IFNAMSIZ - 1);
+        output_bridge[IFNAMSIZ - 1] = '\0';
         *gw = p->gw;
         return true;
     }
@@ -110,7 +111,7 @@ ovs_router_insert__(uint8_t priority, ovs_be32 ip_dst, uint8_t plen,
     rt_init_match(&match, ip_dst, plen);
 
     p = xzalloc(sizeof *p);
-    strncpy(p->output_bridge, output_bridge, IFNAMSIZ);
+    strncpy(p->output_bridge, output_bridge, sizeof(p->output_bridge) - 1);
     p->gw = gw;
     p->nw_addr = match.flow.nw_dst;
     p->plen = plen;
diff --git a/lib/tnl-arp-cache.c b/lib/tnl-arp-cache.c
index 59ec0bc..b3f47d5 100644
--- a/lib/tnl-arp-cache.c
+++ b/lib/tnl-arp-cache.c
@@ -128,7 +128,8 @@ tnl_arp_snoop(const struct flow *flow, struct flow_wildcards *wc,
     arp->ip = flow->nw_src;
     memcpy(arp->mac, flow->arp_sha, ETH_ADDR_LEN);
     arp->expires = time_now() + ARP_ENTRY_DEFAULT_IDLE_TIME;
-    strncpy(arp->br_name, name, IFNAMSIZ);
+    strncpy(arp->br_name, name, sizeof(arp->br_name) - 1);
+    arp->br_name[sizeof(arp->br_name) - 1] = '\0';
     cmap_insert(&table, &arp->cmap_node, (OVS_FORCE uint32_t) arp->ip);
     ovs_mutex_unlock(&mutex);
     return 0;
diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
index e6739b9..da400da 100644
--- a/lib/tnl-ports.c
+++ b/lib/tnl-ports.c
@@ -101,7 +101,7 @@ tnl_port_map_insert(odp_port_t port, ovs_be32 ip_dst, ovs_be16 udp_port,
 
         cls_rule_init(&p->cr, &match, 0);   /* Priority == 0. */
         ovs_refcount_init(&p->ref_cnt);
-        strncpy(p->dev_name, dev_name, IFNAMSIZ);
+        strncpy(p->dev_name, dev_name, sizeof(p->dev_name) - 1);
 
         classifier_insert(&cls, &p->cr, NULL, 0);
     }
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 2f97aa5..13c551c 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2639,7 +2639,8 @@ build_tunnel_send(const struct xlate_ctx *ctx, const struct xport *xport,
         struct xc_entry *entry;
 
         entry = xlate_cache_add_entry(ctx->xin->xcache, XC_TNL_ARP);
-        strncpy(entry->u.tnl_arp_cache.br_name, out_dev->xbridge->name, IFNAMSIZ);
+        strncpy(entry->u.tnl_arp_cache.br_name, out_dev->xbridge->name,
+                sizeof(entry->u.tnl_arp_cache.br_name) - 1);
         entry->u.tnl_arp_cache.d_ip = d_ip;
     }
     err = tnl_port_build_header(xport->ofport, flow,
-- 
2.1.0




More information about the dev mailing list