[ovs-discuss] Possible double-free on ofproto.c:delete_flows_loose

[Ben Pfaff]

Thanks for reporting the bug.

I'll look into backporting the fix.

On Wed, Sep 17, 2014 at 2:17 PM, Anup Khadka <khadka.py at gmail.com> wrote:
> Yes, I got a crash with 100 rules which led me to inspect the code.
>
> The collect_rule function inside collect_rules_loose returns
> OFPROTO_POSTPONE if rule->pending is non-zero (this is possible if the
> ofproto vendor class in not done inserting the rules).
>
> In such cases, collect_rules_loose function will call a free on rules.
>
> Without your commit in July, delete_flows_loose will call free again on the
> rules structure.
>
> This is what was happening in my code base too. I was using OVS 2.1.
>
> Thanks,
> Anup
>
>
>
>
>
>
> On Wed, Sep 17, 2014 at 5:01 PM, Ben Pfaff <blp at nicira.com> wrote:
>>
>> According to the commit message, the bug could not cause a real problem
>> in practice.  Do you see a way that it could?
>>
>> On Wed, Sep 17, 2014 at 04:50:04PM -0400, Anup Khadka wrote:
>> > Looks like this code was added in July:
>> > https://github.com/openvswitch/ovs/commit/bfd3dbf6a0c978ceb20faf292bca51
>> > 3a63e2b68c
>> > I was using an older code-base.
>> > Thanks,
>> > Anup
>> >
>> > On Wed, Sep 17, 2014 at 4:37 PM, Ben Pfaff <blp at nicira.com> wrote:
>> >
>> > > On Wed, Sep 17, 2014 at 02:58:50PM -0400, Anup Khadka wrote:
>> > > > On Tue, Sep 16, 2014 at 3:30 PM, Anup Khadka <khadka.py at gmail.com>
>> > > wrote:
>> > > >
>> > > > > It looks like OVS tries to double-free in delete_flows_loose if
>> > > > > the
>> > > > > rules->rules (inside struct rule_collection *rules is not equal to
>> > > > > rules->stub).
>> > > > >
>> > > > > A little more detail:
>> > > > > In the function delete_flows_loose, the call to the function
>> > > > > collect_rules_loose takes care of freeing rules (again struct
>> > > > > rule_collection *rules) if there is any error while collecting the
>> > > rule.
>> > > > >
>> > > > > The function returns back to delete_flows_loose where it calls
>> > > > > rule_collection_destroy again.
>> > > > >
>> > > > > Because rules->rules is still not rules->stab, it attempts to free
>> > > > > the
>> > > > > rules structure again, resulting in a double-free.
>> > > > >
>> > > > > Perhaps rules->rules can be set to rules->stab inside
>> > > > > rule_collection_destroy function after its freed. Or perhaps,
>> > > > > rule_collection_destroy should only be called from
>> > > > > delete_flows_loose
>> > > if
>> > > > > there is no error, or perhaps collect_rules_loose should not take
>> > > > > care
>> > > of
>> > > > > freeing the data structure.
>> > >
>> > > rule_collection_destroy() already reinitializes 'rules' after it
>> > > destroys it:
>> > >
>> > > void
>> > > rule_collection_destroy(struct rule_collection *rules)
>> > > {
>> > >     if (rules->rules != rules->stub) {
>> > >         free(rules->rules);
>> > >     }
>> > >
>> > >     /* Make repeated destruction harmless. */
>> > >     rule_collection_init(rules);
>> > > }
>> > >
>
>



-- 
"I don't normally do acked-by's.  I think it's my way of avoiding
getting blamed when it all blows up."               Andrew Morton