[ovs-dev] [PATCH branch-1.4] ofproto-dpif: Fix memory leak sending packets to controller.

[Zoltan Kiss]

I've reviewed and tested this. In the patch description there are one 
small mistakes:

-execute_controller_action(), by passing true instead of false as its
+execute_controller_action(), by passing false instead of true as its

But more importantly, todo in handle_miss_upcalls are freed twice, and 
used after the first free, which cause segfaults. After modifying the 
third chunk it worked:

@@ -2710,14 +2709,10 @@ handle_miss_upcalls(struct ofproto_dpif
      /* Process each element in the to-do list, constructing the set of
       * operations to batch. */
      n_ops = 0;
-    HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
+    HMAP_FOR_EACH (miss, hmap_node, &todo) {
          handle_flow_miss(ofproto, miss, flow_miss_ops, &n_ops);
-        ofpbuf_list_delete(&miss->packets);
-        hmap_remove(&todo, &miss->hmap_node);
-        free(miss);
      }
      assert(n_ops <= ARRAY_SIZE(flow_miss_ops));
-    hmap_destroy(&todo);

      /* Execute batch. */
      for (i = 0; i < n_ops; i++) {

Zoli

On 23/04/13 18:16, Ben Pfaff wrote:
> One plausible solution seemed to be to turn over ownership of the packet to
> execute_controller_action(), by passing true instead of false as its
> 'clone' argument.  Another is to add an else case to the "if" statement
> that calls execute_controller_action() to free the packet.
> @@ -2710,11 +2709,8 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>       /* Process each element in the to-do list, constructing the set of
>        * operations to batch. */
>       n_ops = 0;
> -    HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> +    HMAP_FOR_EACH (miss, hmap_node, &todo) {
>           handle_flow_miss(ofproto, miss, flow_miss_ops, &n_ops);
> -        ofpbuf_list_delete(&miss->packets);
> -        hmap_remove(&todo, &miss->hmap_node);
> -        free(miss);
>       }
>       assert(n_ops <= ARRAY_SIZE(flow_miss_ops));
>       hmap_destroy(&todo);