ofpbuf: Update msg when resizing ofpbuf.

Commit 6fd6ed7 (ofpbuf: Simplify ofpbuf API.) introduced the
'header' and 'msg' pointers to 'struct ofpbuf'.  However, we
forget to update the 'msg' pointer when resizing ofpbuf.

This bug could cause serious issue.  For example, in the function
ofputil_encode_nx_packet_in(), the 'msg' pointer is populated in
ofpraw_alloc_xid() when creating the ofpbuf .  Later, the ofpbuf
memory can be reallocated due to the writing to the ofpbuf.
However, since the 'msg' pointer is not updated, the later use of
the 'ofpbuf->msg' will end up writing to either free'ed memory or
memory allocated for other struct.

This commit fixes the bug by always updating the 'header' and
'msg' pointers when the ofpbuf is resized.  Also, a simple test
is added.

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

diff --git a/lib/ofpbuf.c b/lib/ofpbuf.c
index c27c552..05b513c 100644 (file)
--- a/lib/ofpbuf.c
+++ b/lib/ofpbuf.c
@@ -258,9 +258,14 @@ ofpbuf_resize__(struct ofpbuf *b, size_t new_headroom, size_t new_tailroom)
     new_data = (char *) new_base + new_headroom;
     if (b->data != new_data) {
         if (b->header) {
-            uintptr_t data_delta = (char *) new_data - (char *) b->data;
+            uintptr_t data_delta = (char *) b->header - (char *) b->data;
 
-            b->header = (char *) b->header + data_delta;
+            b->header = (char *) new_data + data_delta;
+        }
+        if (b->msg) {
+            uintptr_t data_delta = (char *) b->msg - (char *) b->data;
+
+            b->msg = (char *) new_data + data_delta;
         }
         b->data = new_data;
     }
@@ -292,7 +297,13 @@ ofpbuf_prealloc_headroom(struct ofpbuf *b, size_t size)
  * tailroom to 0, if any.
  *
  * Buffers not obtained from malloc() are not resized, since that wouldn't save
- * any memory. */
+ * any memory.
+ *
+ * Caller needs to updates 'b->header' and 'b->msg' so that they point to the
+ * same locations in the data.  (If they pointed into the tailroom or headroom
+ * then they become invalid.)
+ *
+ */
 void
 ofpbuf_trim(struct ofpbuf *b)
 {
@@ -315,7 +326,11 @@ ofpbuf_padto(struct ofpbuf *b, size_t length)
 /* Shifts all of the data within the allocated space in 'b' by 'delta' bytes.
  * For example, a 'delta' of 1 would cause each byte of data to move one byte
  * forward (from address 'p' to 'p+1'), and a 'delta' of -1 would cause each
- * byte to move one byte backward (from 'p' to 'p-1'). */
+ * byte to move one byte backward (from 'p' to 'p-1').
+ *
+ * If used, user must make sure the 'header' and 'msg' pointers are updated
+ * after shifting.
+ */
 void
 ofpbuf_shift(struct ofpbuf *b, int delta)
 {
@@ -454,6 +469,8 @@ ofpbuf_steal_data(struct ofpbuf *b)
     }
     b->base = NULL;
     b->data = NULL;
+    b->header = NULL;
+    b->msg = NULL;
     return p;
 }
 

diff --git a/lib/ofpbuf.h b/lib/ofpbuf.h
index 80b0dc4..b30cbdb 100644 (file)
--- a/lib/ofpbuf.h
+++ b/lib/ofpbuf.h
@@ -43,6 +43,10 @@ enum OVS_PACKED_ENUM ofpbuf_source {
  *    When parsing, the 'data' will move past these, as data is being
  *    pulled from the OpenFlow message.
  *
+ *    Caution: buffer manipulation of 'struct ofpbuf' must always update
+ *             the 'header' and 'msg' pointers.
+ *
+ *
  * Actions: When encoding OVS action lists, the 'header' is used
  *    as a pointer to the beginning of the current action (see ofpact_put()).
  *

diff --git a/tests/.gitignore b/tests/.gitignore
index 966fcb3..9d0be33 100644 (file)
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -29,6 +29,7 @@
 /test-multipath
 /test-netflow
 /test-odp
+/test-ofpbuf
 /test-ovsdb
 /test-packets
 /test-random

diff --git a/tests/automake.mk b/tests/automake.mk
index 44b528a..2cb93f6 100644 (file)
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -142,6 +142,7 @@ valgrind_wrappers = \
        tests/valgrind/test-lockfile \
        tests/valgrind/test-multipath \
        tests/valgrind/test-odp \
+       tests/valgrind/test-ofpbuf \
        tests/valgrind/test-ovsdb \
        tests/valgrind/test-packets \
        tests/valgrind/test-random \
@@ -280,6 +281,7 @@ tests_ovstest_SOURCES = \
        tests/test-multipath.c \
        tests/test-netflow.c \
        tests/test-odp.c \
+       tests/test-ofpbuf.c \
        tests/test-packets.c \
        tests/test-random.c \
        tests/test-reconnect.c \

diff --git a/tests/library.at b/tests/library.at
index 9bd6d81..6e04991 100644 (file)
--- a/tests/library.at
+++ b/tests/library.at
@@ -209,3 +209,7 @@ AT_CLEANUP
 AT_SETUP([use of public headers])
 AT_CHECK([test-lib], [0], [])
 AT_CLEANUP
+
+AT_SETUP([test ofpbuf module])
+AT_CHECK([ovstest test-ofpbuf], [0], [])
+AT_CLEANUP

diff --git a/tests/test-ofpbuf.c b/tests/test-ofpbuf.c
new file mode 100644 (file)
index 0000000..d88fefe
--- /dev/null
+++ b/tests/test-ofpbuf.c
@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2015 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <config.h>
+#undef NDEBUG
+#include <stdio.h>
+#include "ofpbuf.h"
+#include "ovstest.h"
+#include "util.h"
+
+#define BUF_SIZE 100
+#define HDR_OFS 10
+#define MSG_OFS 50
+
+static void
+test_ofpbuf_main(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
+{
+    struct ofpbuf *buf = ofpbuf_new(BUF_SIZE);
+    int exit_code = 0;
+
+    /* Init checks. */
+    ovs_assert(!buf->size);
+    ovs_assert(buf->allocated == BUF_SIZE);
+    ovs_assert(buf->base == buf->data);
+
+    /* Sets 'buf->header' and 'buf->msg'. */
+    buf->header = (char *) buf->base + HDR_OFS;
+    buf->msg = (char *) buf->base + MSG_OFS;
+
+    /* Gets another 'BUF_SIZE' bytes headroom. */
+    ofpbuf_prealloc_headroom(buf, BUF_SIZE);
+    ovs_assert(!buf->size);
+    ovs_assert(buf->allocated == 2 * BUF_SIZE);
+    ovs_assert((char *) buf->base + BUF_SIZE == buf->data);
+    /* Now 'buf->header' and 'buf->msg' must be BUF_SIZE away from
+     * their original offsets. */
+    ovs_assert(buf->header == (char *) buf->base + BUF_SIZE + HDR_OFS);
+    ovs_assert(buf->msg == (char *) buf->base + BUF_SIZE + MSG_OFS);
+
+    /* Gets another 'BUF_SIZE' bytes tailroom. */
+    ofpbuf_prealloc_tailroom(buf, BUF_SIZE);
+    /* Must remain unchanged. */
+    ovs_assert(!buf->size);
+    ovs_assert(buf->allocated == 2 * BUF_SIZE);
+    ovs_assert((char *) buf->base + BUF_SIZE == buf->data);
+    ovs_assert(buf->header == (char *) buf->base + BUF_SIZE + HDR_OFS);
+    ovs_assert(buf->msg == (char *) buf->base + BUF_SIZE + MSG_OFS);
+
+    ofpbuf_delete(buf);
+    exit(exit_code);
+}
+
+OVSTEST_REGISTER("test-ofpbuf", test_ofpbuf_main);