Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1246?usp=email
to review the following change.
Change subject: buffer: Fix buf_parse eating input
......................................................................
buffer: Fix buf_parse eating input
When parsing a "line" that is longer than the
available line buffer, then buf_parse was
eating up to 2 characters. It advanced past
them but they were not part of the output.
This can lead to unexpected results if buf_parse
is used in a while loop on unrestricted input,
like e.g. when reading configs (see in_src_get()
used for check_inline_file_via_buf()).
Change-Id: I3724660bf0f8336ee58c172acfb7c4f38e457393
Signed-off-by: Frank Lichtenheld <[email protected]>
---
M src/openvpn/buffer.c
M src/openvpn/buffer.h
M tests/unit_tests/openvpn/test_buffer.c
3 files changed, 73 insertions(+), 7 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/46/1246/1
diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index c0b85b2..28de00f 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -832,19 +832,24 @@
do
{
- c = buf_read_u8(buf);
+ c = buf_peek_u8(buf);
if (c < 0)
{
eol = true;
+ line[n] = 0;
+ break;
}
- if (c <= 0 || c == delim)
+ if (c == delim)
{
- c = 0;
+ buf_advance(buf, 1);
+ line[n] = 0;
+ break;
}
- if (n >= size)
+ if (n >= (size - 1))
{
break;
}
+ buf_advance(buf, 1);
line[n++] = (char)c;
} while (c);
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
index 1405667..148cee0 100644
--- a/src/openvpn/buffer.h
+++ b/src/openvpn/buffer.h
@@ -771,7 +771,7 @@
}
static inline int
-buf_read_u8(struct buffer *buf)
+buf_peek_u8(struct buffer *buf)
{
int ret;
if (BLEN(buf) < 1)
@@ -779,7 +779,17 @@
return -1;
}
ret = *BPTR(buf);
- buf_advance(buf, 1);
+ return ret;
+}
+
+static inline int
+buf_read_u8(struct buffer *buf)
+{
+ int ret = buf_peek_u8(buf);
+ if (ret >= 0)
+ {
+ buf_advance(buf, 1);
+ }
return ret;
}
diff --git a/tests/unit_tests/openvpn/test_buffer.c
b/tests/unit_tests/openvpn/test_buffer.c
index c86c19e..92c4c65 100644
--- a/tests/unit_tests/openvpn/test_buffer.c
+++ b/tests/unit_tests/openvpn/test_buffer.c
@@ -450,6 +450,56 @@
gc_free(&gc);
}
+/* for building long texts */
+#define A_TIMES_256
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAO"
+
+void
+test_buffer_parse(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(1024, &gc);
+ char line[512];
+ bool status;
+ const char test1[] = A_TIMES_256 "EOL\n" A_TIMES_256 "EOF";
+
+ /* line buffer bigger than actual line */
+ assert_int_equal(buf_write(&buf, test1, sizeof(test1)), true);
+ status = buf_parse(&buf, '\n', line, sizeof(line));
+ assert_int_equal(status, true);
+ assert_string_equal(line, A_TIMES_256 "EOL");
+ status = buf_parse(&buf, '\n', line, sizeof(line));
+ assert_int_equal(status, true);
+ assert_string_equal(line, A_TIMES_256 "EOF");
+
+ /* line buffer exactly same size as actual line + terminating \0 */
+ buf_reset_len(&buf);
+ assert_int_equal(buf_write(&buf, test1, sizeof(test1)), true);
+ status = buf_parse(&buf, '\n', line, 260);
+ assert_int_equal(status, true);
+ assert_string_equal(line, A_TIMES_256 "EOL");
+ status = buf_parse(&buf, '\n', line, 260);
+ assert_int_equal(status, true);
+ assert_string_equal(line, A_TIMES_256 "EOF");
+
+ /* line buffer smaller than actual line */
+ buf_reset_len(&buf);
+ assert_int_equal(buf_write(&buf, test1, sizeof(test1)), true);
+ status = buf_parse(&buf, '\n', line, 257);
+ assert_int_equal(status, true);
+ assert_string_equal(line, A_TIMES_256);
+ status = buf_parse(&buf, '\n', line, 257);
+ assert_int_equal(status, true);
+ assert_string_equal(line, "EOL");
+ status = buf_parse(&buf, '\n', line, 257);
+ assert_int_equal(status, true);
+ assert_string_equal(line, A_TIMES_256);
+ status = buf_parse(&buf, '\n', line, 257);
+ assert_int_equal(status, true);
+ assert_string_equal(line, "EOF");
+
+ gc_free(&gc);
+}
+
int
main(void)
{
@@ -478,7 +528,8 @@
cmocka_unit_test(test_character_class),
cmocka_unit_test(test_character_string_mod_buf),
cmocka_unit_test(test_snprintf),
- cmocka_unit_test(test_buffer_chomp)
+ cmocka_unit_test(test_buffer_chomp),
+ cmocka_unit_test(test_buffer_parse)
};
return cmocka_run_group_tests_name("buffer", tests, NULL, NULL);
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1246?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I3724660bf0f8336ee58c172acfb7c4f38e457393
Gerrit-Change-Number: 1246
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
