All new accounts created on Gitlab now require administrator approval. If you invite any collaborators, please let Flux staff know so they can approve the accounts.

Commit 8dd3f945 authored by Alex Wang's avatar Alex Wang

ofpbuf: Update msg when resizing ofpbuf.

Commit 6fd6ed71 (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: default avatarAlex Wang <alexw@nicira.com>
Acked-by: default avatarBen Pfaff <blp@nicira.com>
parent d1d1d3e9
......@@ -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;
}
......
......@@ -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()).
*
......
......@@ -29,6 +29,7 @@
/test-multipath
/test-netflow
/test-odp
/test-ofpbuf
/test-ovsdb
/test-packets
/test-random
......
......@@ -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 \
......
......@@ -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
/*
* 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);
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment