PATCH: ctdb: buffer write beyond limits
Christof Schmitt
cs at samba.org
Tue Feb 19 21:02:31 UTC 2019
On Tue, Feb 19, 2019 at 07:06:14PM +0100, swen wrote:
> On Tue, 2019-02-19 at 09:48 -0700, Christof Schmitt via samba-technical
> wrote:
> > On Tue, Feb 19, 2019 at 12:37:18PM +0100, swen via samba-technical
> > wrote:
> > > On Tue, 2019-02-19 at 11:53 +0100, Volker Lendecke wrote:
> > > > On Tue, Feb 19, 2019 at 11:02:18AM +0100, Volker Lendecke via
> > > > samba-
> > > > technical wrote:
> > > > > On Tue, Feb 19, 2019 at 10:53:41AM +0100, swen wrote:
> > > > > > the code is right if the following facts are taken into
> > > > > > account.
> > > > > > 1. MAX_SIGNED_INT = MAX_UNSIGNED_INT/2 - 1
> > > > > > 2. length is defined as unsigned int and will get assigned at
> > > > > > most MAX_SIGNED_INT(num_ready)
> > > > > > 3. offset is initially zero and will only grow as much as
> > > > > > length
> > > > > > is shrinking (=>code)
> > > > > > => therefore, offset + length are at most
> > > > > > MAX_SIGNED_INT.
> > > > > > Guaranteed !
> > > > > > 4. If then another (guaranteed positive) signed integer (here
> > > > > > num_ready) is added we simply
> > > > > > cannot overflow an unsigned int because
> > > > > > MAX_SIGNED_INT + MAX_SIGNED_INT = MAX_UNSIGNED_INT - 2
> > > > > >
> > > > > > Taken those simple maths into account, I hope you can agree
> > > > > > to
> > > > > > the code.
> > > > >
> > > > > No, I do not agree. There is a simple boiler plate to do a
> > > > > checked
> > > > > addition, and a simple boiler plate to do checked subtraction.
> > > > > Please
> > > > > do it that way.
> > > >
> > > > More explanation: I do not agree because it is really simple to
> > > > get
> > > > these lines right on their own respect. And having to read a 4-
> > > > entry
> > > > proof referencing code elsewhere where it's easy to make to code
> > > > safe
> > > > in an isolated manner.
> > > >
> > > > The crash was introduced through patches that did not affect this
> > > > MIN
> > > > statement at all. This means that the environment can change, and
> > > > the
> > > > assumptions that you refer to in bullet point 3 might change,
> > > > causing
> > > > the code to crash again. In this kind of code we have to check
> > > > every
> > > > individual arithmetic operation in isolation.
> > > >
> > > > Attached find something that I did in 10 minutes. Please crush it
> > > > if
> > > > you think your approach is better.
> >
> > Considering the complexity here: Would it be possible to add unit
> > tests
> > here? First to ensure that the fix addresses the problem at hand and
> > then also to stress other corner cases. I am not yet sure what the
> > best
> > strategy here would be. It could be unit testing the function
> > (cmocka?) or
> > setting up a test client that sends the combinations of requests to
> > ctdb
> > that we want to test. Either way, we should ensure that this works
> > and
> > does not regress with future patches.
>
> What made me believe that this could be an easy fix for an obvious issue ?
>
> The code, as it is now, is wrong. This fix is a must !
> The additional checks/verifications around the main fixing-code
> are negotiable (to me) but I was hoping to finally have
> the comprehensive patch at hand now.
>
> Sorry Christof but where is the complexity and
> which corner cases could you think of ?
>
> Adding tests is never a bad idea but considering where we are in the release phase
> could we agree on a fix for the base-/understood-issue first ?
> I'm getting the impression that the blanket solution might take a few more iterations.
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?
Christof
-------------- next part --------------
From b078c000eaae43bceefe6e439d9c1ca167361652 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] ctdb-tests: Add test for ctdb_io.c
Signed-off-by: Christof Schmitt <cs at samba.org>
---
ctdb/tests/cunit/ctdb_io_test_001.sh | 7 ++
ctdb/tests/src/ctdb_io_test.c | 112 +++++++++++++++++++++++++++
2 files changed, 119 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..6415085c3df
--- /dev/null
+++ b/ctdb/tests/cunit/ctdb_io_test_001.sh
@@ -0,0 +1,7 @@
+#!/bin/sh
+
+. "${TEST_SCRIPTS_DIR}/unit.sh"
+
+ok_null
+
+unit_test ctdb_io_test 1
diff --git a/ctdb/tests/src/ctdb_io_test.c b/ctdb/tests/src/ctdb_io_test.c
new file mode 100644
index 00000000000..28a15336557
--- /dev/null
+++ b/ctdb/tests/src/ctdb_io_test.c
@@ -0,0 +1,112 @@
+/*
+ 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, length) == 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);
+}
+
+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;
+
+ default:
+ fprintf(stderr, "Unknown test number %s\n", argv[1]);
+ }
+
+ return 0;
+}
--
2.17.0
More information about the samba-technical
mailing list