[ovs-dev,1/9] ofproto/trace: Fix memory leak in oftrace_push_ct_state()

Free the allocated memory in the pop function.

Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 ofproto/ofproto-dpif-trace.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

On 08/25/2017 03:51 PM, Yi-Hung Wei wrote:
> Free the allocated memory in the pop function.
>
> Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
>   ofproto/ofproto-dpif-trace.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index 38d11002f290..a45c9cfd9619 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -128,12 +128,14 @@ oftrace_push_ct_state(struct ovs_list *next_ct_states, uint32_t ct_state)
>       ovs_list_push_back(next_ct_states, &next_ct_state->node);
>   }
>
> -static uint32_t
> -oftrace_pop_ct_state(struct ovs_list *next_ct_states)
> +static void
> +oftrace_pop_ct_state(struct ovs_list *next_ct_states, uint32_t *ct_state)
>   {
>       struct oftrace_next_ct_state *s;
>       LIST_FOR_EACH_POP (s, node, next_ct_states) {
> -        return s->state;
> +        *ct_state = s->state;
> +        free(s);
> +        return;
>       }
>       OVS_NOT_REACHED();
>   }
> @@ -404,7 +406,8 @@ static void
>   free_ct_states(struct ovs_list *ct_states)
>   {
>       while (!ovs_list_is_empty(ct_states)) {
> -        oftrace_pop_ct_state(ct_states);
> +        uint32_t state;

Maybe a blank line here?

> +        oftrace_pop_ct_state(ct_states, &state);
>       }
>   }
>
> @@ -646,7 +649,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
>                   ds_put_cstr(output, " - resume conntrack with default "
>                               "ct_state=trk|new (use --ct-next to customize)");
>               } else {
> -                ct_state = oftrace_pop_ct_state(next_ct_states);
> +                oftrace_pop_ct_state(next_ct_states, &ct_state);
>                   struct ds s = DS_EMPTY_INITIALIZER;
>                   format_flags(&s, ct_state_to_string, ct_state, '|');
>                   ds_put_format(output, " - resume conntrack with ct_state=%s",
>
Besides the minor nit above it LGTM.

Reviewed-by: Greg Rose <gvrose8192@gmail.com>

On Fri, Aug 25, 2017 at 03:51:11PM -0700, Yi-Hung Wei wrote:
> Free the allocated memory in the pop function.
> 
> Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> ---
>  ofproto/ofproto-dpif-trace.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index 38d11002f290..a45c9cfd9619 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -128,12 +128,14 @@ oftrace_push_ct_state(struct ovs_list *next_ct_states, uint32_t ct_state)
>      ovs_list_push_back(next_ct_states, &next_ct_state->node);
>  }
>  
> -static uint32_t
> -oftrace_pop_ct_state(struct ovs_list *next_ct_states)
> +static void
> +oftrace_pop_ct_state(struct ovs_list *next_ct_states, uint32_t *ct_state)
>  {
>      struct oftrace_next_ct_state *s;
>      LIST_FOR_EACH_POP (s, node, next_ct_states) {
> -        return s->state;
> +        *ct_state = s->state;
> +        free(s);
> +        return;
>      }
>      OVS_NOT_REACHED();
>  }

Thanks for the fix!

I don't understand why the function return type needs to change.  Can
you change this to preserve the return type, while fixing the memory
leak?

Thanks,

Ben.

Thanks Greg and Ben for review. It looks like this patch is
independent to the reset of the series. I will pull it out and send
v2.

Thanks,

-Yi-Hung

On Tue, Oct 31, 2017 at 3:18 PM, Ben Pfaff <blp@ovn.org> wrote:
> On Fri, Aug 25, 2017 at 03:51:11PM -0700, Yi-Hung Wei wrote:
>> Free the allocated memory in the pop function.
>>
>> Fixes: 0f2f05bbcf743 ("ofproto/trace: Add --ct-next option to ofproto/trace")
>> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>> ---
>>  ofproto/ofproto-dpif-trace.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
>> index 38d11002f290..a45c9cfd9619 100644
>> --- a/ofproto/ofproto-dpif-trace.c
>> +++ b/ofproto/ofproto-dpif-trace.c
>> @@ -128,12 +128,14 @@ oftrace_push_ct_state(struct ovs_list *next_ct_states, uint32_t ct_state)
>>      ovs_list_push_back(next_ct_states, &next_ct_state->node);
>>  }
>>
>> -static uint32_t
>> -oftrace_pop_ct_state(struct ovs_list *next_ct_states)
>> +static void
>> +oftrace_pop_ct_state(struct ovs_list *next_ct_states, uint32_t *ct_state)
>>  {
>>      struct oftrace_next_ct_state *s;
>>      LIST_FOR_EACH_POP (s, node, next_ct_states) {
>> -        return s->state;
>> +        *ct_state = s->state;
>> +        free(s);
>> +        return;
>>      }
>>      OVS_NOT_REACHED();
>>  }
>
> Thanks for the fix!
>
> I don't understand why the function return type needs to change.  Can
> you change this to preserve the return type, while fixing the memory
> leak?
>
> Thanks,
>
> Ben.