[ovs-dev,1/2] ofp-parse: Fix small memory leak when calling parse_ofp_meter_mod_str().
diff mbox

Message ID 1498610129-32658-1-git-send-email-jpettit@ovn.org
State Accepted
Headers show

Commit Message

Justin Pettit June 28, 2017, 12:35 a.m. UTC
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(-)

Comments

Justin Pettit June 28, 2017, 1:03 a.m. UTC | #1
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

Patch
diff mbox

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