From: Jarno Rajahalme <jrajahalme@nicira.com>
Date: Tue, 17 Dec 2013 23:54:30 +0000 (-0800)
Subject: Do not free uninitialized packets.
X-Git-Url: http://git.cascardo.eti.br/?p=cascardo%2Fovs.git;a=commitdiff_plain;h=837a88dccb2634c26f3a70af37b86cae8a48ad74

Do not free uninitialized packets.

Commit da546e0 (dpif: Allow execute to modify the packet.) uninitializes
the "dpif_upcall.packet" of "struct upcall" when dpif_recv() returns error.
The packet ofpbuf is likely uninitialized in this case, hence calling
ofpbuf_uninit() on it will likely cause a SEGFAULT.

This commit fixes this bug by only uninitializing packet's ofpbuf on
successfully received upcalls.

A note warning about this is added on the comment of dpif_recv() in
dpif.c and dpif-provider.h.

Reported-by: Alex Wang <alexw@nicira.com>
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
---

diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index f279934b6..90a02b436 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -349,7 +349,9 @@ struct dpif_class {
      * The caller owns the data of 'upcall->packet' and may modify it.  If
      * packet's headroom is exhausted as it is manipulated, 'upcall->packet'
      * will be reallocated.  This requires the data of 'upcall->packet' to be
-     * released with ofpbuf_uninit() before 'upcall' is destroyed.
+     * released with ofpbuf_uninit() before 'upcall' is destroyed.  However,
+     * when an error is returned, the 'upcall->packet' may be uninitialized
+     * and should not be released.
      *
      * This function must not block.  If no upcall is pending when it is
      * called, it should return EAGAIN without blocking. */
diff --git a/lib/dpif.c b/lib/dpif.c
index 7220b380c..6b519b8bd 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1315,6 +1315,13 @@ dpif_recv_set(struct dpif *dpif, bool enable)
  * 'upcall->key' and 'upcall->userdata' point into data in the caller-provided
  * 'buf', so their memory cannot be freed separately from 'buf'.
  *
+ * The caller owns the data of 'upcall->packet' and may modify it.  If
+ * packet's headroom is exhausted as it is manipulated, 'upcall->packet'
+ * will be reallocated.  This requires the data of 'upcall->packet' to be
+ * released with ofpbuf_uninit() before 'upcall' is destroyed.  However,
+ * when an error is returned, the 'upcall->packet' may be uninitialized
+ * and should not be released.
+ *
  * Returns 0 if successful, otherwise a positive errno value.  Returns EAGAIN
  * if no upcall is immediately available. */
 int
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 12214d298..f0088f0ea 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -533,7 +533,10 @@ recv_upcalls(struct udpif *udpif)
         error = dpif_recv(udpif->dpif, &upcall->dpif_upcall,
                           &upcall->upcall_buf);
         if (error) {
-            upcall_destroy(upcall);
+            /* upcall_destroy() can only be called on successfully received
+             * upcalls. */
+            ofpbuf_uninit(&upcall->upcall_buf);
+            free(upcall);
             break;
         }