From f5a2e2319bc7c3cce7f5cdccae44b9bae3e33375 Mon Sep 17 00:00:00 2001 From: Frank Lichtenheld Date: Wed, 8 Oct 2025 12:29:55 +0200 Subject: [PATCH] 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 Acked-by: Gert Doering Acked-by: Arne Schwabe Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1246 Message-Id: <20251008103001.7696-1-gert@greenie.muc.de> URL: https://sourceforge.net/p/openvpn/mailman/message/59243829/ Signed-off-by: Gert Doering --- src/openvpn/buffer.c | 13 +++++-- src/openvpn/buffer.h | 14 ++++++- tests/unit_tests/openvpn/test_buffer.c | 53 +++++++++++++++++++++++++- 3 files changed, 73 insertions(+), 7 deletions(-) diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index c0b85b210..28de00fdb 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -832,19 +832,24 @@ buf_parse(struct buffer *buf, const int delim, char *line, const int size) 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 140566784..148cee061 100644 --- a/src/openvpn/buffer.h +++ b/src/openvpn/buffer.h @@ -771,7 +771,7 @@ buf_read(struct buffer *src, void *dest, int size) } 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 @@ buf_read_u8(struct buffer *buf) 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 c86c19e5a..92c4c6586 100644 --- a/tests/unit_tests/openvpn/test_buffer.c +++ b/tests/unit_tests/openvpn/test_buffer.c @@ -450,6 +450,56 @@ test_buffer_chomp(void **state) 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 @@ main(void) 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); -- 2.47.3