From patchwork Sat Jul 1 02:53:58 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steffan Karger X-Patchwork-Id: 101 Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director1.mail.ord1d.rsapps.net ([172.30.191.6]) by backend31.mail.ord1d.rsapps.net (Dovecot) with LMTP id w9woHF7SH1pxQAAAgoeIoA for ; Thu, 30 Nov 2017 04:41:50 -0500 Received: from proxy8.mail.ord1d.rsapps.net ([172.30.191.6]) by director1.mail.ord1d.rsapps.net (Dovecot) with LMTP id rQaJBF7SH1rJbwAANGzteQ ; Thu, 30 Nov 2017 04:41:50 -0500 Received: from smtp33.gate.ord1d ([172.30.191.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by proxy8.mail.ord1d.rsapps.net (Dovecot) with LMTP id oQ9fBV7SH1qNKwAAGdz6CA ; Thu, 30 Nov 2017 04:41:50 -0500 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO Authentication-Results: smtp33.gate.ord1d.rsapps.net x-tls.subject="/OU=GT58646928/OU=See www.geotrust.com/resources/cps (c)14/OU=Domain Control Validated - QuickSSL(R) Premium/CN=ns2.fox-it.com"; auth=pass (cipher=DHE-RSA-AES256-SHA) X-Virus-Scanned: OK X-Orig-To: patchwork@openvpn.net X-Originating-Ip: [178.250.144.131] Authentication-Results: smtp33.gate.ord1d.rsapps.net; iprev=pass policy.iprev="178.250.144.131"; spf=pass smtp.mailfrom="steffan.karger@fox-it.com" smtp.helo="ns2.fox-it.com"; dkim=none (message not signed) header.d=none; dmarc=none (p=nil; dis=none) header.from=fox-it.com X-Classification-ID: af8c1006-d5b2-11e7-ade5-525400041ef2-1-1 Received: from [178.250.144.131] ([178.250.144.131:42983] helo=ns2.fox-it.com) by smtp33.gate.ord1d.rsapps.net (envelope-from ) (ecelerity 4.2.1.56364 r(Core:4.2.1.14)) with ESMTPS (cipher=DHE-RSA-AES256-SHA subject="/OU=GT58646928/OU=See www.geotrust.com/resources/cps (c)14/OU=Domain Control Validated - QuickSSL(R) Premium/CN=ns2.fox-it.com") id 53/2A-11456-D52DF1A5; Thu, 30 Nov 2017 04:41:49 -0500 Received: from FOXDFT52.FOX.local (unknown [10.0.0.129]) by ns2.fox-it.com (Postfix) with ESMTPS id 290D11C50A4 for ; Thu, 30 Nov 2017 10:41:48 +0100 (CET) Received: from steffan-fox (172.16.5.186) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Thu, 30 Nov 2017 10:41:47 +0100 Resent-From: Steffan Karger Resent-Date: Thu, 30 Nov 2017 10:41:35 +0100 Resent-Message-ID: <20171130094135.GA5754@steffan-fox> Resent-To: Received: from FOXDFT52.FOX.local (10.0.0.129) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2 via Mailbox Transport; Sat, 1 Jul 2017 14:54:09 +0200 Received: from steffan-fox.fox.local (172.16.5.170) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Sat, 1 Jul 2017 14:54:09 +0200 From: Steffan Karger To: CC: Steffan Karger Subject: [PATCH 1/5] buffer_list_aggregate_separator(): add unit tests Date: Sat, 1 Jul 2017 14:53:58 +0200 Message-ID: <1498913642-32459-1-git-send-email-steffan.karger@fox-it.com> X-Mailer: git-send-email 2.7.4 X-ClientProxiedBy: FOXDFT52.FOX.local (10.0.0.129) To FOXDFT52.FOX.local (10.0.0.129) MIME-Version: 1.0 X-ClientProxiedBy: FOXDFT52.FOX.local (10.0.0.129) To FOXDFT52.FOX.local (10.0.0.129) X-getmail-retrieved-from-mailbox: Inbox Before refactoring this function, add tests to verify the new implementation. While writing the tests, it became clear that this function is not behaving very well. We'll fix that in follow-up commits. Signed-off-by: Steffan Karger --- tests/unit_tests/openvpn/test_buffer.c | 181 ++++++++++++++++++++++++++++++++- 1 file changed, 179 insertions(+), 2 deletions(-) diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c index 69bb2e5..acfeb1a 100644 --- a/tests/unit_tests/openvpn/test_buffer.c +++ b/tests/unit_tests/openvpn/test_buffer.c @@ -35,7 +35,7 @@ #include "buffer.h" static void -buffer_strprefix(void **state) +test_buffer_strprefix(void **state) { assert_true(strprefix("123456", "123456")); assert_true(strprefix("123456", "123")); @@ -44,11 +44,188 @@ buffer_strprefix(void **state) assert_false(strprefix("12", "123")); } +#define testsep "," +#define testnosep "" +#define teststr1 "one" +#define teststr2 "two" +#define teststr3 "three" +#define teststr4 "four" + +struct test_buffer_list_aggregate_ctx { + struct buffer_list *empty; + struct buffer_list *one_two_three; + struct buffer_list *zero_length_strings; + struct buffer_list *empty_buffers; +}; + +static int test_buffer_list_setup(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = calloc(1, sizeof(*ctx)); + ctx->empty = buffer_list_new(0); + + ctx->one_two_three = buffer_list_new(3); + buffer_list_push(ctx->one_two_three, teststr1); + buffer_list_push(ctx->one_two_three, teststr2); + buffer_list_push(ctx->one_two_three, teststr3); + + ctx->zero_length_strings = buffer_list_new(2); + buffer_list_push(ctx->zero_length_strings, ""); + buffer_list_push(ctx->zero_length_strings, ""); + + ctx->empty_buffers = buffer_list_new(2); + uint8_t *data1 = malloc(1); + uint8_t *data2 = malloc(1); + buffer_list_push_data(ctx->empty_buffers, data1, 0); + buffer_list_push_data(ctx->empty_buffers, data2, 0); + + *state = ctx; + return 0; +} + +static int test_buffer_list_teardown(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + + buffer_list_free(ctx->empty); + buffer_list_free(ctx->one_two_three); + buffer_list_free(ctx->zero_length_strings); + buffer_list_free(ctx->empty_buffers); + free(ctx); + return 0; +} + +static void +test_buffer_list_full(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + + /* list full */ + assert_int_equal(ctx->one_two_three->size, 3); + buffer_list_push(ctx->one_two_three, teststr4); + assert_int_equal(ctx->one_two_three->size, 3); +} + +static void +test_buffer_list_aggregate_separator_empty(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + + /* aggregating an empty buffer list results in an empty buffer list */ + buffer_list_aggregate_separator(ctx->empty, 3, testsep); + assert_null(ctx->empty->head); +} + +static void +test_buffer_list_aggregate_separator_noop(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + + /* With a max length of 2, no aggregation should take place */ + buffer_list_aggregate_separator(ctx->one_two_three, 2, testsep); + assert_int_equal(ctx->one_two_three->size, 3); + struct buffer *buf = buffer_list_peek(ctx->one_two_three); + assert_string_equal((const char *)buf->data, teststr1); +} + +static void +test_buffer_list_aggregate_separator_two(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + + /* Aggregate the first two elements */ + /* FIXME this exceeds the supplied max */ + buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep); + /* FIXME size does not get adjusted after aggregating */ + assert_int_equal(ctx->one_two_three->size, 3); + struct buffer *buf = buffer_list_peek(ctx->one_two_three); + assert_string_equal((const char *)buf->data, + teststr1 testsep teststr2 testsep); +} + +static void +test_buffer_list_aggregate_separator_all(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + + /* Aggregate all */ + buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testsep); + /* FIXME size does not get adjusted after aggregating */ + assert_int_equal(ctx->one_two_three->size, 3); + struct buffer *buf = buffer_list_peek(ctx->one_two_three); + assert_string_equal((const char *)buf->data, + teststr1 testsep teststr2 testsep teststr3 testsep); +} + +static void +test_buffer_list_aggregate_separator_nosep(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + + /* Aggregate all */ + buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testnosep); + /* FIXME size does not get adjusted after aggregating */ + assert_int_equal(ctx->one_two_three->size, 3); + struct buffer *buf = buffer_list_peek(ctx->one_two_three); + assert_string_equal((const char *)buf->data, teststr1 teststr2 teststr3); +} + +static void +test_buffer_list_aggregate_separator_zerolen(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + struct buffer_list *bl_zerolen = ctx->zero_length_strings; + + /* Aggregate all */ + buffer_list_aggregate_separator(bl_zerolen, 1<<16, testnosep); + /* FIXME size does not get adjusted after aggregating */ + assert_int_equal(bl_zerolen->size, 2); + struct buffer *buf = buffer_list_peek(bl_zerolen); + assert_string_equal((const char *)buf->data, ""); +} + +static void +test_buffer_list_aggregate_separator_emptybuffers(void **state) +{ + struct test_buffer_list_aggregate_ctx *ctx = *state; + struct buffer_list *bl_emptybuffers = ctx->empty_buffers; + + /* Aggregate all */ + buffer_list_aggregate_separator(bl_emptybuffers, 1<<16, testnosep); + /* FIXME size does not get adjusted after aggregating */ + assert_int_equal(bl_emptybuffers->size, 2); + struct buffer *buf = buffer_list_peek(bl_emptybuffers); + assert_int_equal(BLEN(buf), 0); +} + int main(void) { const struct CMUnitTest tests[] = { - cmocka_unit_test(buffer_strprefix), + cmocka_unit_test(test_buffer_strprefix), + cmocka_unit_test_setup_teardown(test_buffer_list_full, + test_buffer_list_setup, + test_buffer_list_teardown), + cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_empty, + test_buffer_list_setup, + test_buffer_list_teardown), + cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_noop, + test_buffer_list_setup, + test_buffer_list_teardown), + cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_two, + test_buffer_list_setup, + test_buffer_list_teardown), + cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_all, + test_buffer_list_setup, + test_buffer_list_teardown), + cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_nosep, + test_buffer_list_setup, + test_buffer_list_teardown), + cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_zerolen, + test_buffer_list_setup, + test_buffer_list_teardown), + cmocka_unit_test_setup_teardown(test_buffer_list_aggregate_separator_emptybuffers, + test_buffer_list_setup, + test_buffer_list_teardown), }; return cmocka_run_group_tests_name("buffer", tests, NULL, NULL); From patchwork Sat Jul 1 02:53:59 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steffan Karger X-Patchwork-Id: 103 Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director5.mail.ord1d.rsapps.net ([172.28.255.1]) by backend31.mail.ord1d.rsapps.net (Dovecot) with LMTP id g5XWDhzTH1qWKQAAgoeIoA for ; Thu, 30 Nov 2017 04:45:00 -0500 Received: from director4.mail.ord1c.rsapps.net ([172.28.255.1]) by director5.mail.ord1d.rsapps.net (Dovecot) with LMTP id 243HDhzTH1rNXwAAsdCWiw ; Thu, 30 Nov 2017 04:45:00 -0500 Received: from smtp23.gate.ord1a ([172.28.255.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by director4.mail.ord1c.rsapps.net (Dovecot) with LMTP id UG8eJxzTH1oKbAAAsEL7Xg ; Thu, 30 Nov 2017 04:45:00 -0500 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO Authentication-Results: smtp23.gate.ord1a.rsapps.net x-tls.subject="/OU=GT58646928/OU=See www.geotrust.com/resources/cps (c)14/OU=Domain Control Validated - QuickSSL(R) Premium/CN=ns2.fox-it.com"; auth=pass (cipher=DHE-RSA-AES256-SHA) X-Virus-Scanned: OK X-Orig-To: patchwork@openvpn.net X-Originating-Ip: [178.250.144.131] Authentication-Results: smtp23.gate.ord1a.rsapps.net; iprev=pass policy.iprev="178.250.144.131"; spf=pass smtp.mailfrom="steffan.karger@fox-it.com" smtp.helo="ns2.fox-it.com"; dkim=none (message not signed) header.d=none; dmarc=none (p=nil; dis=none) header.from=fox-it.com X-Classification-ID: 200f75c0-d5b3-11e7-b745-90e6ba3f2eba-1-1 Received: from [178.250.144.131] ([178.250.144.131:16940] helo=ns2.fox-it.com) by smtp23.gate.ord1a.rsapps.net (envelope-from ) (ecelerity 4.2.1.56364 r(Core:4.2.1.14)) with ESMTPS (cipher=DHE-RSA-AES256-SHA subject="/OU=GT58646928/OU=See www.geotrust.com/resources/cps (c)14/OU=Domain Control Validated - QuickSSL(R) Premium/CN=ns2.fox-it.com") id E8/92-14794-A13DF1A5; Thu, 30 Nov 2017 04:44:58 -0500 Received: from FOXDFT52.FOX.local (unknown [10.0.0.129]) by ns2.fox-it.com (Postfix) with ESMTPS id F0CDB1C50A4 for ; Thu, 30 Nov 2017 10:44:56 +0100 (CET) Received: from steffan-fox (172.16.5.186) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Thu, 30 Nov 2017 10:44:56 +0100 Resent-From: Steffan Karger Resent-Date: Thu, 30 Nov 2017 10:44:55 +0100 Resent-Message-ID: <20171130094455.GD5754@steffan-fox> Resent-To: Received: from FOXDFT52.FOX.local (10.0.0.129) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2 via Mailbox Transport; Sat, 1 Jul 2017 14:54:12 +0200 Received: from steffan-fox.fox.local (172.16.5.170) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Sat, 1 Jul 2017 14:54:12 +0200 From: Steffan Karger To: CC: Steffan Karger Subject: [PATCH 2/5] buffer_list_aggregate_separator(): update list size after aggregating Date: Sat, 1 Jul 2017 14:53:59 +0200 Message-ID: <1498913642-32459-2-git-send-email-steffan.karger@fox-it.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1498913642-32459-1-git-send-email-steffan.karger@fox-it.com> References: <1498913642-32459-1-git-send-email-steffan.karger@fox-it.com> X-ClientProxiedBy: FOXDFT52.FOX.local (10.0.0.129) To FOXDFT52.FOX.local (10.0.0.129) MIME-Version: 1.0 X-ClientProxiedBy: FOXDFT52.FOX.local (10.0.0.129) To FOXDFT52.FOX.local (10.0.0.129) X-getmail-retrieved-from-mailbox: Inbox After aggregating a buffer_list, the size should be adjusted accordingly. Signed-off-by: Steffan Karger --- src/openvpn/buffer.c | 1 + tests/unit_tests/openvpn/test_buffer.c | 15 +++++---------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index 87e27ec..1cc2957 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -1265,6 +1265,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max, const e = next; } bl->head = f; + bl->size -= count-1; f->next = more; if (!more) { diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c index acfeb1a..6842114 100644 --- a/tests/unit_tests/openvpn/test_buffer.c +++ b/tests/unit_tests/openvpn/test_buffer.c @@ -135,8 +135,7 @@ test_buffer_list_aggregate_separator_two(void **state) /* Aggregate the first two elements */ /* FIXME this exceeds the supplied max */ buffer_list_aggregate_separator(ctx->one_two_three, 4, testsep); - /* FIXME size does not get adjusted after aggregating */ - assert_int_equal(ctx->one_two_three->size, 3); + assert_int_equal(ctx->one_two_three->size, 2); struct buffer *buf = buffer_list_peek(ctx->one_two_three); assert_string_equal((const char *)buf->data, teststr1 testsep teststr2 testsep); @@ -149,8 +148,7 @@ test_buffer_list_aggregate_separator_all(void **state) /* Aggregate all */ buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testsep); - /* FIXME size does not get adjusted after aggregating */ - assert_int_equal(ctx->one_two_three->size, 3); + assert_int_equal(ctx->one_two_three->size, 1); struct buffer *buf = buffer_list_peek(ctx->one_two_three); assert_string_equal((const char *)buf->data, teststr1 testsep teststr2 testsep teststr3 testsep); @@ -163,8 +161,7 @@ test_buffer_list_aggregate_separator_nosep(void **state) /* Aggregate all */ buffer_list_aggregate_separator(ctx->one_two_three, 1<<16, testnosep); - /* FIXME size does not get adjusted after aggregating */ - assert_int_equal(ctx->one_two_three->size, 3); + assert_int_equal(ctx->one_two_three->size, 1); struct buffer *buf = buffer_list_peek(ctx->one_two_three); assert_string_equal((const char *)buf->data, teststr1 teststr2 teststr3); } @@ -177,8 +174,7 @@ test_buffer_list_aggregate_separator_zerolen(void **state) /* Aggregate all */ buffer_list_aggregate_separator(bl_zerolen, 1<<16, testnosep); - /* FIXME size does not get adjusted after aggregating */ - assert_int_equal(bl_zerolen->size, 2); + assert_int_equal(bl_zerolen->size, 1); struct buffer *buf = buffer_list_peek(bl_zerolen); assert_string_equal((const char *)buf->data, ""); } @@ -191,8 +187,7 @@ test_buffer_list_aggregate_separator_emptybuffers(void **state) /* Aggregate all */ buffer_list_aggregate_separator(bl_emptybuffers, 1<<16, testnosep); - /* FIXME size does not get adjusted after aggregating */ - assert_int_equal(bl_emptybuffers->size, 2); + assert_int_equal(bl_emptybuffers->size, 1); struct buffer *buf = buffer_list_peek(bl_emptybuffers); assert_int_equal(BLEN(buf), 0); } From patchwork Sat Jul 1 02:54:01 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steffan Karger X-Patchwork-Id: 104 Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director2.mail.ord1d.rsapps.net ([172.30.191.6]) by backend31.mail.ord1d.rsapps.net (Dovecot) with LMTP id czqLFR3TH1pxQAAAgoeIoA for ; Thu, 30 Nov 2017 04:45:01 -0500 Received: from proxy17.mail.ord1d.rsapps.net ([172.30.191.6]) by director2.mail.ord1d.rsapps.net (Dovecot) with LMTP id c/mAFR3TH1q7XwAAgYhSiA ; Thu, 30 Nov 2017 04:45:01 -0500 Received: from smtp24.gate.ord1d ([172.30.191.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by proxy17.mail.ord1d.rsapps.net (Dovecot) with LMTP id QnpeFB3TH1ooLQAAWC7mWg ; Thu, 30 Nov 2017 04:45:01 -0500 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO Authentication-Results: smtp24.gate.ord1d.rsapps.net x-tls.subject="/OU=GT58646928/OU=See www.geotrust.com/resources/cps (c)14/OU=Domain Control Validated - QuickSSL(R) Premium/CN=ns2.fox-it.com"; auth=pass (cipher=DHE-RSA-AES256-SHA) X-Virus-Scanned: OK X-Orig-To: patchwork@openvpn.net X-Originating-Ip: [178.250.144.131] Authentication-Results: smtp24.gate.ord1d.rsapps.net; iprev=pass policy.iprev="178.250.144.131"; spf=pass smtp.mailfrom="steffan.karger@fox-it.com" smtp.helo="ns2.fox-it.com"; dkim=none (message not signed) header.d=none; dmarc=none (p=nil; dis=none) header.from=fox-it.com X-Classification-ID: 21b6435e-d5b3-11e7-9bf5-52540091a1c4-1-1 Received: from [178.250.144.131] ([178.250.144.131:8294] helo=ns2.fox-it.com) by smtp24.gate.ord1d.rsapps.net (envelope-from ) (ecelerity 4.2.1.56364 r(Core:4.2.1.14)) with ESMTPS (cipher=DHE-RSA-AES256-SHA subject="/OU=GT58646928/OU=See www.geotrust.com/resources/cps (c)14/OU=Domain Control Validated - QuickSSL(R) Premium/CN=ns2.fox-it.com") id 71/86-17519-C13DF1A5; Thu, 30 Nov 2017 04:45:01 -0500 Received: from FOXDFT52.FOX.local (unknown [10.0.0.129]) by ns2.fox-it.com (Postfix) with ESMTPS id B05841C50A4 for ; Thu, 30 Nov 2017 10:44:59 +0100 (CET) Received: from steffan-fox (172.16.5.186) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Thu, 30 Nov 2017 10:44:59 +0100 Resent-From: Steffan Karger Resent-Date: Thu, 30 Nov 2017 10:44:58 +0100 Resent-Message-ID: <20171130094458.GF5754@steffan-fox> Resent-To: Received: from FOXDFT52.FOX.local (10.0.0.129) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2 via Mailbox Transport; Sat, 1 Jul 2017 14:54:13 +0200 Received: from steffan-fox.fox.local (172.16.5.170) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Sat, 1 Jul 2017 14:54:13 +0200 From: Steffan Karger To: CC: Steffan Karger Subject: [PATCH 4/5] buffer_list_aggregate_separator(): prevent 0-byte malloc Date: Sat, 1 Jul 2017 14:54:01 +0200 Message-ID: <1498913642-32459-4-git-send-email-steffan.karger@fox-it.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1498913642-32459-1-git-send-email-steffan.karger@fox-it.com> References: <1498913642-32459-1-git-send-email-steffan.karger@fox-it.com> X-ClientProxiedBy: FOXDFT52.FOX.local (10.0.0.129) To FOXDFT52.FOX.local (10.0.0.129) MIME-Version: 1.0 X-ClientProxiedBy: FOXDFT52.FOX.local (10.0.0.129) To FOXDFT52.FOX.local (10.0.0.129) X-getmail-retrieved-from-mailbox: Inbox As pointed out in finding OVPN-05 of the cryptograpy engineering audit (funded by Private Internet Access), buffer_list_aggregate_separator() could perform a 0-byte malloc when called with a list of 0-length buffers and a "" separator. If other could would later try to access that buffer memory, this would result in undefined behaviour. To prevent this, always malloc() 1 byte. To simplify as we go, use alloc_buf() to allocate the buffer. This has the additional benefit that the actual buffer data (not the contents) is zero-terminated, because alloc_buf() calls calloc() and we have 1 extra byte of data. Signed-off-by: Steffan Karger --- src/openvpn/buffer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index 2700ad1..ecbdc1b 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -1261,8 +1261,7 @@ buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, struct buffer_entry *e = bl->head, *f; ALLOC_OBJ_CLEAR(f, struct buffer_entry); - f->buf.data = malloc(size); - check_malloc_return(f->buf.data); + f->buf = alloc_buf(size+1); /* prevent 0-byte malloc */ f->buf.capacity = size; for (i = 0; e && i < count; ++i) { From patchwork Sat Jul 1 02:54:02 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Steffan Karger X-Patchwork-Id: 105 Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director2.mail.ord1d.rsapps.net ([172.28.255.1]) by backend31.mail.ord1d.rsapps.net (Dovecot) with LMTP id S7TGLSDTH1qWKQAAgoeIoA for ; Thu, 30 Nov 2017 04:45:04 -0500 Received: from director4.mail.ord1c.rsapps.net ([172.28.255.1]) by director2.mail.ord1d.rsapps.net (Dovecot) with LMTP id 69xUAiDTH1rAXwAAgYhSiA ; Thu, 30 Nov 2017 04:45:04 -0500 Received: from smtp47.gate.ord1a ([172.28.255.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by director4.mail.ord1c.rsapps.net (Dovecot) with LMTP id SC3tCyHTH1rUawAAsEL7Xg ; Thu, 30 Nov 2017 04:45:05 -0500 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO Authentication-Results: smtp47.gate.ord1a.rsapps.net x-tls.subject="/OU=GT58646928/OU=See www.geotrust.com/resources/cps (c)14/OU=Domain Control Validated - QuickSSL(R) Premium/CN=ns2.fox-it.com"; auth=pass (cipher=DHE-RSA-AES256-SHA) X-Virus-Scanned: OK X-Orig-To: patchwork@openvpn.net X-Originating-Ip: [178.250.144.131] Authentication-Results: smtp47.gate.ord1a.rsapps.net; iprev=pass policy.iprev="178.250.144.131"; spf=pass smtp.mailfrom="steffan.karger@fox-it.com" smtp.helo="ns2.fox-it.com"; dkim=none (message not signed) header.d=none; dmarc=none (p=nil; dis=none) header.from=fox-it.com X-Classification-ID: 22629294-d5b3-11e7-8682-0022192d87f6-1-1 Received: from [178.250.144.131] ([178.250.144.131:15937] helo=ns2.fox-it.com) by smtp47.gate.ord1a.rsapps.net (envelope-from ) (ecelerity 4.2.1.56364 r(Core:4.2.1.14)) with ESMTPS (cipher=DHE-RSA-AES256-SHA subject="/OU=GT58646928/OU=See www.geotrust.com/resources/cps (c)14/OU=Domain Control Validated - QuickSSL(R) Premium/CN=ns2.fox-it.com") id 03/DF-19269-E13DF1A5; Thu, 30 Nov 2017 04:45:02 -0500 Received: from FOXDFT52.FOX.local (unknown [10.0.0.129]) by ns2.fox-it.com (Postfix) with ESMTPS id D1A0B1C50A5 for ; Thu, 30 Nov 2017 10:45:00 +0100 (CET) Received: from steffan-fox (172.16.5.186) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Thu, 30 Nov 2017 10:45:00 +0100 Resent-From: Steffan Karger Resent-Date: Thu, 30 Nov 2017 10:44:59 +0100 Resent-Message-ID: <20171130094459.GG5754@steffan-fox> Resent-To: Received: from FOXDFT52.FOX.local (10.0.0.129) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2 via Mailbox Transport; Sat, 1 Jul 2017 14:54:13 +0200 Received: from steffan-fox.fox.local (172.16.5.170) by FOXDFT52.FOX.local (10.0.0.129) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Sat, 1 Jul 2017 14:54:13 +0200 From: Steffan Karger To: CC: Steffan Karger Subject: [PATCH 5/5] buffer_list_aggregate_separator(): simplify code Date: Sat, 1 Jul 2017 14:54:02 +0200 Message-ID: <1498913642-32459-5-git-send-email-steffan.karger@fox-it.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1498913642-32459-1-git-send-email-steffan.karger@fox-it.com> References: <1498913642-32459-1-git-send-email-steffan.karger@fox-it.com> X-ClientProxiedBy: FOXDFT52.FOX.local (10.0.0.129) To FOXDFT52.FOX.local (10.0.0.129) MIME-Version: 1.0 X-ClientProxiedBy: FOXDFT52.FOX.local (10.0.0.129) To FOXDFT52.FOX.local (10.0.0.129) X-getmail-retrieved-from-mailbox: Inbox Clean up the function by slightly simplifying the logic. Mostly witespace changes, so best reviewed using 'git diff -w'. Signed-off-by: Steffan Karger --- src/openvpn/buffer.c | 75 ++++++++++++++++++++++++---------------------------- 1 file changed, 35 insertions(+), 40 deletions(-) diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index ecbdc1b..abb2342 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -1234,51 +1234,46 @@ void buffer_list_aggregate_separator(struct buffer_list *bl, const size_t max_len, const char *sep) { - int sep_len = strlen(sep); + const int sep_len = strlen(sep); + struct buffer_entry *more = bl->head; + size_t size = 0; + int count = 0; + for (count = 0; more; ++count) + { + size_t extra_len = BLEN(&more->buf) + sep_len; + if (size + extra_len <= max_len) + { + size += extra_len; + more = more->next; + } + else + { + break; + } + } - if (bl->head) + if (count >= 2) { - struct buffer_entry *more = bl->head; - size_t size = 0; - int count = 0; - for (count = 0; more; ++count) + struct buffer_entry *f; + ALLOC_OBJ_CLEAR(f, struct buffer_entry); + f->buf = alloc_buf(size+1); /* prevent 0-byte malloc */ + + struct buffer_entry *e = bl->head; + for (size_t i = 0; e && i < count; ++i) { - size_t extra_len = BLEN(&more->buf) + sep_len; - if (size + extra_len <= max_len) - { - size += extra_len; - more = more->next; - } - else - { - break; - } + struct buffer_entry *next = e->next; + buf_copy(&f->buf, &e->buf); + buf_write(&f->buf, sep, sep_len); + free_buf(&e->buf); + free(e); + e = next; } - - if (count >= 2) + bl->head = f; + bl->size -= count-1; + f->next = more; + if (!more) { - int i; - struct buffer_entry *e = bl->head, *f; - - ALLOC_OBJ_CLEAR(f, struct buffer_entry); - f->buf = alloc_buf(size+1); /* prevent 0-byte malloc */ - f->buf.capacity = size; - for (i = 0; e && i < count; ++i) - { - struct buffer_entry *next = e->next; - buf_copy(&f->buf, &e->buf); - buf_write(&f->buf, sep, sep_len); - free_buf(&e->buf); - free(e); - e = next; - } - bl->head = f; - bl->size -= count-1; - f->next = more; - if (!more) - { - bl->tail = f; - } + bl->tail = f; } } }