PATCH: ctdb: buffer write beyond limits
Christof Schmitt
cs at samba.org
Wed Feb 20 20:46:14 UTC 2019
On Wed, Feb 20, 2019 at 02:11:18PM +1100, Martin Schwenke via samba-technical wrote:
> On Wed, 20 Feb 2019 13:59:15 +1100, Martin Schwenke via samba-technical
> <samba-technical at lists.samba.org> wrote:
>
> > On Tue, 19 Feb 2019 16:17:30 -0700, Christof Schmitt via
> > samba-technical <samba-technical at lists.samba.org> wrote:
> >
> > > On Tue, Feb 19, 2019 at 02:02:31PM -0700, Christof Schmitt via samba-technical wrote:
> > > > Maybe we can get this in another day? See the attached patch for the
> > > > start of a test of the queue code. Could we extend that to trigger the
> > > > problem and then verify that it is fixed with your patch?
> > >
> > > The initial patch was missing the wscript update. Here is an extended
> > > version that shows the memory corruption without your fix. I would
> > > suggest to add the fix with a test now to fix the 4.10 release and then
> > > we can work towards adding more tests for the queueing code.
> >
> > Thanks for that. That looks really good. I always thought we would
> > have problems writing unit tests for that code because it uses struct
> > ctdb_context, which drags in the world, but this is nice!
> >
> > Ironically, the test refuses to compile with CFLAGS="-O3" and
> > --picky-developer:
> >
> > [318/380] Compiling ctdb/tests/src/ctdb_io_test.c
> > In file included from ../../tests/src/ctdb_io_test.c:23:
> > ../../tests/src/ctdb_io_test.c: In function ‘test1_callback’:
> > ../../tests/src/ctdb_io_test.c:70:9: error: ‘__builtin_memcmp_eq’ reading 12 bytes from a region of size 9 [-Werror=stringop-overflow=]
> > assert(memcmp(data + sizeof(len), test1_req, len) == 0);
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> >
> > That is, the test reads off the end of the buffer... :-)
> >
> > The fixup below seems to do the job:
The irony... Thanks you for the fix, that looks correct.
> >
> > diff --git a/ctdb/tests/src/ctdb_io_test.c b/ctdb/tests/src/ctdb_io_test.c
> > index 22bd2f4930f..5a2f81538a1 100644
> > --- a/ctdb/tests/src/ctdb_io_test.c
> > +++ b/ctdb/tests/src/ctdb_io_test.c
> > @@ -67,7 +67,7 @@ static void test1_callback(uint8_t *data, size_t length, void *private_data)
> > assert(len == sizeof(uint32_t) + test1_req_len);
> >
> > assert(length == sizeof(uint32_t) + test1_req_len);
> > - assert(memcmp(data + sizeof(len), test1_req, length) == 0);
> > + assert(memcmp(data + sizeof(len), test1_req, test1_req_len) == 0);
> > }
> >
> > static void test1(void)
>
> Oh, and if we're going to push it with the bug fix then can we please
> add the bug tag?
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13791
Yes, here are the fix with my reviewed-by and the updated test patch.
Besides your fix i also had to fix the dependency of the new test on
tdb; otherwise a compile on a system without tdb.h fails.
I started a pipeline run at
https://gitlab.com/samba-team/devel/samba/pipelines/48450894
Christof
-------------- next part --------------
From fa1ab7a3b9e48cb8bff8850bcfe6947208a60f0c Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Fri, 15 Feb 2019 14:34:05 +0100
Subject: [PATCH 1/2] ctdb: buffer write beyond limits
In order to calculate the number of bytes correctly which
are to be read into the buffer, the buffer.offset must be taken
into account.
This patch fixes a regression introduced by 382705f495dd.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13791
Signed-off-by: Swen Schillig <swen at linux.ibm.com>
Reviewed-by: Christof Schmitt <cs at samba.org>
---
ctdb/common/ctdb_io.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/ctdb/common/ctdb_io.c b/ctdb/common/ctdb_io.c
index d86540762ea..c16eb7f67b7 100644
--- a/ctdb/common/ctdb_io.c
+++ b/ctdb/common/ctdb_io.c
@@ -164,6 +164,7 @@ static void queue_io_read(struct ctdb_queue *queue)
{
int num_ready = 0;
uint32_t pkt_size = 0;
+ uint32_t start_offset;
ssize_t nread;
uint8_t *data;
@@ -226,7 +227,17 @@ buffer_shift:
}
data_read:
- num_ready = MIN(num_ready, queue->buffer.size - queue->buffer.length);
+ start_offset = queue->buffer.length + queue->buffer.offset;
+ if (start_offset < queue->buffer.length) {
+ DBG_ERR("Buffer overflow\n");
+ goto failed;
+ }
+ if (start_offset > queue->buffer.size) {
+ DBG_ERR("Buffer overflow\n");
+ goto failed;
+ }
+
+ num_ready = MIN(num_ready, queue->buffer.size - start_offset);
if (num_ready > 0) {
nread = sys_read(queue->fd,
--
2.17.0
From 7614448c4b62fad62bd2040eeab3e9afb51603e8 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Tue, 19 Feb 2019 13:59:05 -0700
Subject: [PATCH 2/2] ctdb-tests: Add test for ctdb_io.c
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13791
Signed-off-by: Christof Schmitt <cs at samba.org>
---
ctdb/tests/cunit/ctdb_io_test_001.sh | 8 ++
ctdb/tests/src/ctdb_io_test.c | 196 +++++++++++++++++++++++++++
ctdb/wscript | 6 +
3 files changed, 210 insertions(+)
create mode 100755 ctdb/tests/cunit/ctdb_io_test_001.sh
create mode 100644 ctdb/tests/src/ctdb_io_test.c
diff --git a/ctdb/tests/cunit/ctdb_io_test_001.sh b/ctdb/tests/cunit/ctdb_io_test_001.sh
new file mode 100755
index 00000000000..15842ea8e17
--- /dev/null
+++ b/ctdb/tests/cunit/ctdb_io_test_001.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+. "${TEST_SCRIPTS_DIR}/unit.sh"
+
+ok_null
+
+unit_test ctdb_io_test 1
+unit_test ctdb_io_test 2
diff --git a/ctdb/tests/src/ctdb_io_test.c b/ctdb/tests/src/ctdb_io_test.c
new file mode 100644
index 00000000000..5a2f81538a1
--- /dev/null
+++ b/ctdb/tests/src/ctdb_io_test.c
@@ -0,0 +1,196 @@
+/*
+ ctdb_io tests
+
+ Copyright (C) Christof Schmitt 2019
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include "replace.h"
+#include "system/filesys.h"
+
+#include <assert.h>
+
+#include "common/ctdb_io.c"
+
+void ctdb_set_error(struct ctdb_context *ctdb, const char *fmt, ...)
+{
+ va_list ap;
+ va_start(ap, fmt);
+ vprintf(fmt, ap);
+ assert(false);
+}
+
+static void test_setup(ctdb_queue_cb_fn_t cb,
+ int *pfd,
+ struct ctdb_context **pctdb)
+{
+ int pipefd[2], ret;
+ struct ctdb_context *ctdb;
+ struct ctdb_queue *queue;
+
+ ret = pipe(pipefd);
+ assert(ret == 0);
+
+ ctdb = talloc_zero(NULL, struct ctdb_context);
+ assert(ctdb != NULL);
+
+ ctdb->ev = tevent_context_init(NULL);
+
+ queue = ctdb_queue_setup(ctdb, ctdb, pipefd[0], 0, cb,
+ NULL, "test queue");
+ assert(queue != NULL);
+
+ *pctdb = ctdb;
+ *pfd = pipefd[1];
+}
+
+static const size_t test1_req_len = 8;
+static const char *test1_req = "abcdefgh";
+
+static void test1_callback(uint8_t *data, size_t length, void *private_data)
+{
+ uint32_t len;
+
+ len = *(uint32_t *)data;
+ assert(len == sizeof(uint32_t) + test1_req_len);
+
+ assert(length == sizeof(uint32_t) + test1_req_len);
+ assert(memcmp(data + sizeof(len), test1_req, test1_req_len) == 0);
+}
+
+static void test1(void)
+{
+ struct ctdb_context *ctdb;
+ int fd, ret;
+ uint32_t pkt_size;
+
+ test_setup(test1_callback, &fd, &ctdb);
+
+ pkt_size = sizeof(uint32_t) + test1_req_len;
+ ret = write(fd, &pkt_size, sizeof(pkt_size));
+ assert(ret == sizeof(pkt_size));
+
+ ret = write(fd, test1_req, test1_req_len);
+ assert(ret == test1_req_len);
+
+ tevent_loop_once(ctdb->ev);
+
+ TALLOC_FREE(ctdb);
+}
+
+static const size_t test2_req_len[] = { 900, 24, 600 };
+
+static int test2_cb_num = 0;
+
+static void test2_callback(uint8_t *data, size_t length, void *private_data)
+{
+ uint32_t len;
+
+ len = *(uint32_t *)data;
+ assert(len == sizeof(uint32_t) + test2_req_len[test2_cb_num]);
+ assert(length == sizeof(uint32_t) + test2_req_len[test2_cb_num]);
+
+ test2_cb_num++;
+}
+
+static void test2(void)
+{
+ struct ctdb_context *ctdb;
+ int fd, ret, i;
+ uint32_t pkt_size;
+ char req[1024] = { 0 };
+
+ for (i = 0; i < sizeof(req); i++) {
+ req[i] = i % CHAR_MAX;
+ }
+
+ test_setup(test2_callback, &fd, &ctdb);
+
+ /*
+ * request 0
+ */
+
+ pkt_size = sizeof(uint32_t) + test2_req_len[0];
+ ret = write(fd, &pkt_size, sizeof(pkt_size));
+ assert(ret == sizeof(pkt_size));
+
+ ret = write(fd, req, test2_req_len[0]);
+ assert(ret == test2_req_len[0]);
+
+ /*
+ * request 1
+ */
+ pkt_size = sizeof(uint32_t) + test2_req_len[1];
+ ret = write(fd, &pkt_size, sizeof(pkt_size));
+ assert(ret == sizeof(pkt_size));
+
+ /*
+ * Omit the last byte to avoid buffer processing.
+ */
+ ret = write(fd, req, test2_req_len[1] - 1);
+ assert(ret == test2_req_len[1] - 1);
+
+ tevent_loop_once(ctdb->ev);
+
+ /*
+ * Write the missing byte now.
+ */
+ ret = write(fd, &req[test2_req_len[1] - 1], 1);
+ assert(ret == 1);
+
+ /*
+ * request 2
+ */
+ pkt_size = sizeof(uint32_t) + test2_req_len[2];
+ ret = write(fd, &pkt_size, sizeof(pkt_size));
+ assert(ret == sizeof(pkt_size));
+
+ ret = write(fd, req, test2_req_len[2]);
+ assert(ret == test2_req_len[2]);
+
+ tevent_loop_once(ctdb->ev);
+ tevent_loop_once(ctdb->ev);
+
+ assert(test2_cb_num == 2);
+
+ TALLOC_FREE(ctdb);
+}
+
+int main(int argc, const char **argv)
+{
+ int num;
+
+ if (argc != 2) {
+ fprintf(stderr, "%s <testnum>\n", argv[0]);
+ exit(1);
+ }
+
+
+ num = atoi(argv[1]);
+ switch (num) {
+ case 1:
+ test1();
+ break;
+
+ case 2:
+ test2();
+ break;
+
+ default:
+ fprintf(stderr, "Unknown test number %s\n", argv[1]);
+ }
+
+ return 0;
+}
diff --git a/ctdb/wscript b/ctdb/wscript
index 30b09d6dc16..c2f1956a916 100644
--- a/ctdb/wscript
+++ b/ctdb/wscript
@@ -943,6 +943,12 @@ def build(bld):
LIBASYNC_REQ samba-util sys_rw''',
install_path='${CTDB_TEST_LIBEXECDIR}')
+ bld.SAMBA_BINARY('ctdb_io_test',
+ source='tests/src/ctdb_io_test.c',
+ deps='''talloc tevent tdb samba-util sys_rw''',
+ install_path='${CTDB_TEST_LIBEXECDIR}')
+
+
bld.SAMBA_SUBSYSTEM('ctdb-protocol-tests-basic',
source=bld.SUBDIR('tests/src',
'protocol_common_basic.c'),
--
2.17.0
More information about the samba-technical
mailing list