ofp-util: Make decoding switch features harder to misuse (and fix leak).

Until now, ofputil_decode_switch_features() has put the ports from the
switch features message into a separate ofpbuf supplied as an argument.
The natural desire for a caller is to just reuse an ofpbuf that it already
has, and that's what one of the callers did.  This however has the
nonobvious effect of leaking the memory that the ofpbuf previously owned,
since it gets replaced by an OFPBUF_CONST-type ofpbuf.

This commit avoids the problem by changing the interface to pull the
header from an ofpbuf that the caller already has.

This fixes a leak in testcase 909 "ofproto-dpif - patch ports".

Found by valgrind.

Reported-by: William Tu <u9012063@gmail.com>
Reported-at: http://openvswitch.org/pipermail/dev/2016-January/064771.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Andy Zhou <azhou@ovn.org>

diff --git a/lib/learning-switch.c b/lib/learning-switch.c
index 35c3fef..002b818 100644 (file)
--- a/lib/learning-switch.c
+++ b/lib/learning-switch.c
@@ -419,7 +419,8 @@ process_switch_features(struct lswitch *sw, struct ofp_header *oh)
     enum ofperr error;
     struct ofpbuf b;
 
-    error = ofputil_decode_switch_features(oh, &features, &b);
+    ofpbuf_use_const(&b, oh, ntohs(oh->length));
+    error = ofputil_pull_switch_features(&b, &features);
     if (error) {
         VLOG_ERR("received invalid switch feature reply (%s)",
                  ofperr_to_string(error));

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 5c9a6ab..5af4bf0 100644 (file)
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -458,7 +458,8 @@ ofp_print_switch_features(struct ds *string, const struct ofp_header *oh)
     enum ofperr error;
     struct ofpbuf b;
 
-    error = ofputil_decode_switch_features(oh, &features, &b);
+    ofpbuf_use_const(&b, oh, ntohs(oh->length));
+    error = ofputil_pull_switch_features(&b, &features);
     if (error) {
         ofp_print_error(string, error);
         return;

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index fb0ef3e..aa4d2f3 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4186,23 +4186,18 @@ ofputil_capabilities_mask(enum ofp_version ofp_version)
     }
 }
 
-/* Decodes an OpenFlow 1.0 or 1.1 "switch_features" structure 'osf' into an
- * abstract representation in '*features'.  Initializes '*b' to iterate over
- * the OpenFlow port structures following 'osf' with later calls to
- * ofputil_pull_phy_port().  Returns 0 if successful, otherwise an
- * OFPERR_* value.  */
+/* Pulls an OpenFlow "switch_features" structure from 'b' and decodes it into
+ * an abstract representation in '*features', readying 'b' to iterate over the
+ * OpenFlow port structures following 'osf' with later calls to
+ * ofputil_pull_phy_port().  Returns 0 if successful, otherwise an OFPERR_*
+ * value.  */
 enum ofperr
-ofputil_decode_switch_features(const struct ofp_header *oh,
-                               struct ofputil_switch_features *features,
-                               struct ofpbuf *b)
+ofputil_pull_switch_features(struct ofpbuf *b,
+                             struct ofputil_switch_features *features)
 {
-    const struct ofp_switch_features *osf;
-    enum ofpraw raw;
-
-    ofpbuf_use_const(b, oh, ntohs(oh->length));
-    raw = ofpraw_pull_assert(b);
-
-    osf = ofpbuf_pull(b, sizeof *osf);
+    const struct ofp_header *oh = b->data;
+    enum ofpraw raw = ofpraw_pull_assert(b);
+    const struct ofp_switch_features *osf = ofpbuf_pull(b, sizeof *osf);
     features->datapath_id = ntohll(osf->datapath_id);
     features->n_buffers = ntohl(osf->n_buffers);
     features->n_tables = osf->n_tables;

diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index cf77d8a..866e1bc 100644 (file)
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -590,9 +590,8 @@ struct ofputil_switch_features {
     uint64_t ofpacts;           /* Bitmap of OFPACT_* bits. */
 };
 
-enum ofperr ofputil_decode_switch_features(const struct ofp_header *,
-                                           struct ofputil_switch_features *,
-                                           struct ofpbuf *);
+enum ofperr ofputil_pull_switch_features(struct ofpbuf *,
+                                         struct ofputil_switch_features *);
 
 struct ofpbuf *ofputil_encode_switch_features(
     const struct ofputil_switch_features *, enum ofputil_protocol,

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 1ad48c3..96d6c89 100644 (file)
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -846,7 +846,6 @@ port_iterator_fetch_features(struct port_iterator *pi)
     run(vconn_transact(pi->vconn, rq, &pi->reply),
         "talking to %s", vconn_get_name(pi->vconn));
 
-    const struct ofp_header *oh = pi->reply->data;
     enum ofptype type;
     if (ofptype_decode(&type, pi->reply->data)
         || type != OFPTYPE_FEATURES_REPLY) {
@@ -865,8 +864,7 @@ port_iterator_fetch_features(struct port_iterator *pi)
     }
 
     struct ofputil_switch_features features;
-    enum ofperr error = ofputil_decode_switch_features(oh, &features,
-                                                       pi->reply);
+    enum ofperr error = ofputil_pull_switch_features(pi->reply, &features);
     if (error) {
         ovs_fatal(0, "%s: failed to decode features reply (%s)",
                   vconn_get_name(pi->vconn), ofperr_to_string(error));