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 <[email protected]>
Reported-at: http://openvswitch.org/pipermail/dev/2016-January/064771.html
Signed-off-by: Ben Pfaff <[email protected]>
Acked-by: Andy Zhou <[email protected]>

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));

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;

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;

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,

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));