| Message ID | 1498610129-32658-1-git-send-email-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
This patch has an issue. I'm working on a v2. --Justin > On Jun 27, 2017, at 5:35 PM, Justin Pettit <jpettit@ovn.org> wrote: > > The function parse_ofp_meter_mod_str() allocates a buffer called > 'bands', which parse_ofp_meter_mod_str__() then steals for the member > 'mm->meter.bands'. Calling functions didn't free that stolen value and > the comments for those function didn't indicate that was necessary. > > Found by valgrind. > > Signed-off-by: Justin Pettit <jpettit@ovn.org> > --- > lib/ofp-parse.c | 11 +++++++---- > utilities/ovs-ofctl.c | 2 ++ > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c > index 528b75b4f4e1..725fc41e063c 100644 > --- a/lib/ofp-parse.c > +++ b/lib/ofp-parse.c > @@ -754,6 +754,8 @@ parse_ofp_packet_out_str(struct ofputil_packet_out *po, const char *str_, > return error; > } > > +/* Parse a string representation of a meter modification message to '*mm'. > + * If successful, 'mm->meter.bands' must be free()d by the caller. */ > static char * OVS_WARN_UNUSED_RESULT > parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, > struct ofpbuf *bands, int command, > @@ -795,6 +797,9 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, > mm->command = command; > mm->meter.meter_id = 0; > mm->meter.flags = 0; > + mm->meter.n_bands = 0; > + mm->meter.bands = NULL; > + > if (fields & F_BANDS) { > band_str = strstr(string, "band"); > if (!band_str) { > @@ -945,9 +950,6 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, > } > } > } > - } else { > - mm->meter.n_bands = 0; > - mm->meter.bands = NULL; > } > > return NULL; > @@ -957,7 +959,8 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, > * page) into 'mm' for sending the specified meter_mod 'command' to a switch. > * > * Returns NULL if successful, otherwise a malloc()'d string describing the > - * error. The caller is responsible for freeing the returned string. */ > + * error. The caller is responsible for freeing the returned string. > + * If successful, 'mm->meter.bands' must be free()d by the caller. */ > char * OVS_WARN_UNUSED_RESULT > parse_ofp_meter_mod_str(struct ofputil_meter_mod *mm, const char *str_, > int command, enum ofputil_protocol *usable_protocols) > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c > index ad862efe3e39..200ae9cb0078 100644 > --- a/utilities/ovs-ofctl.c > +++ b/utilities/ovs-ofctl.c > @@ -3704,6 +3704,7 @@ ofctl_meter_mod__(const char *bridge, const char *str, int command) > protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols); > version = ofputil_protocol_to_ofp_version(protocol); > transact_noreply(vconn, ofputil_encode_meter_mod(version, &mm)); > + free(mm.meter.bands); > vconn_close(vconn); > } > > @@ -3732,6 +3733,7 @@ ofctl_meter_request__(const char *bridge, const char *str, > version = ofputil_protocol_to_ofp_version(protocol); > dump_transaction(vconn, ofputil_encode_meter_request(version, type, > mm.meter.meter_id)); > + free(mm.meter.bands); > vconn_close(vconn); > } > > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 528b75b4f4e1..725fc41e063c 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -754,6 +754,8 @@ parse_ofp_packet_out_str(struct ofputil_packet_out *po, const char *str_, return error; } +/* Parse a string representation of a meter modification message to '*mm'. + * If successful, 'mm->meter.bands' must be free()d by the caller. */ static char * OVS_WARN_UNUSED_RESULT parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, struct ofpbuf *bands, int command, @@ -795,6 +797,9 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, mm->command = command; mm->meter.meter_id = 0; mm->meter.flags = 0; + mm->meter.n_bands = 0; + mm->meter.bands = NULL; + if (fields & F_BANDS) { band_str = strstr(string, "band"); if (!band_str) { @@ -945,9 +950,6 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, } } } - } else { - mm->meter.n_bands = 0; - mm->meter.bands = NULL; } return NULL; @@ -957,7 +959,8 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, * page) into 'mm' for sending the specified meter_mod 'command' to a switch. * * Returns NULL if successful, otherwise a malloc()'d string describing the - * error. The caller is responsible for freeing the returned string. */ + * error. The caller is responsible for freeing the returned string. + * If successful, 'mm->meter.bands' must be free()d by the caller. */ char * OVS_WARN_UNUSED_RESULT parse_ofp_meter_mod_str(struct ofputil_meter_mod *mm, const char *str_, int command, enum ofputil_protocol *usable_protocols) diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index ad862efe3e39..200ae9cb0078 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -3704,6 +3704,7 @@ ofctl_meter_mod__(const char *bridge, const char *str, int command) protocol = open_vconn_for_flow_mod(bridge, &vconn, usable_protocols); version = ofputil_protocol_to_ofp_version(protocol); transact_noreply(vconn, ofputil_encode_meter_mod(version, &mm)); + free(mm.meter.bands); vconn_close(vconn); } @@ -3732,6 +3733,7 @@ ofctl_meter_request__(const char *bridge, const char *str, version = ofputil_protocol_to_ofp_version(protocol); dump_transaction(vconn, ofputil_encode_meter_request(version, type, mm.meter.meter_id)); + free(mm.meter.bands); vconn_close(vconn); }
The function parse_ofp_meter_mod_str() allocates a buffer called 'bands', which parse_ofp_meter_mod_str__() then steals for the member 'mm->meter.bands'. Calling functions didn't free that stolen value and the comments for those function didn't indicate that was necessary. Found by valgrind. Signed-off-by: Justin Pettit <jpettit@ovn.org> --- lib/ofp-parse.c | 11 +++++++---- utilities/ovs-ofctl.c | 2 ++ 2 files changed, 9 insertions(+), 4 deletions(-)