From: Max Khon <fjoe@samodelkin.net>
Date: Sun, 30 Oct 2022 22:25:15 +0000 (+0200)
Subject: Try to shift up to sbuff start as current sbuff can be child sbuff with no used space... 
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=049e8b3fe757fe65bcdb8524740c1a0a7bbb8f19;p=thirdparty%2Ffreeradius-server.git

Try to shift up to sbuff start as current sbuff can be child sbuff with no used space (#4761)

* Try to shift up to sbuff start as current sbuff can be child sbuff with no used space.

Co-authored-by: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
---

diff --git a/src/lib/util/sbuff.c b/src/lib/util/sbuff.c
index 223db6206e5..bd2a2e3ba48 100644
--- a/src/lib/util/sbuff.c
+++ b/src/lib/util/sbuff.c
@@ -257,7 +257,7 @@ size_t fr_sbuff_shift(fr_sbuff_t *sbuff, size_t shift)
 size_t fr_sbuff_extend_file(fr_sbuff_t *sbuff, size_t extension)
 {
 	fr_sbuff_t		*sbuff_i;
-	size_t			read, available, total_read;
+	size_t			read, available, total_read, shift;
 	fr_sbuff_uctx_file_t	*fctx;
 
 	CHECK_SBUFF_INIT(sbuff);
@@ -273,14 +273,29 @@ size_t fr_sbuff_extend_file(fr_sbuff_t *sbuff, size_t extension)
 		return 0;	/* There's no way we could satisfy the extension request */
 	}
 
-	if (fr_sbuff_used(sbuff)) {
+	/*
+	 *	Shift out the maximum number of bytes we can
+	 *	irrespective of the amount that was requested
+	 *	as the extension.  It's more efficient to do
+	 *	this than lots of small shifts, and just 
+	 *	looking and the number of bytes used in the
+	 *	deepest sbuff, and using that as the shift
+	 *	amount, might mean we don't shift anything at
+	 *	all!
+	 *
+	 *	fr_sbuff_shift will cap the max shift amount,
+	 *	so markers and positions will remain valid for 
+	 *	all sbuffs in the chain.
+	 */
+	shift = fr_sbuff_current(sbuff) - fr_sbuff_buff(sbuff);
+	if (shift) {
 		/*
 		 *	Try and shift as much as we can out
 		 *	of the buffer to make space.
 		 *
 		 *	Note: p and markers are constraints here.
 		 */
-		fctx->shifted += fr_sbuff_shift(sbuff, fr_sbuff_used(sbuff));
+		fctx->shifted += fr_sbuff_shift(sbuff, shift);
 	}
 
 	available = fctx->buff_end - sbuff->end;
diff --git a/src/lib/util/sbuff_tests.c b/src/lib/util/sbuff_tests.c
index 0d569774535..be77e4af2d9 100644
--- a/src/lib/util/sbuff_tests.c
+++ b/src/lib/util/sbuff_tests.c
@@ -1019,26 +1019,39 @@ static void test_talloc_extend_with_marker(void)
 static void test_file_extend(void)
 {
 	fr_sbuff_t	sbuff;
-	fr_sbuff_t	our_sbuff;
+	fr_sbuff_t	our_sbuff, child_sbuff;
 	fr_sbuff_uctx_file_t	fctx;
 	FILE		*fp;
-	char		buff[16];
+	char		buff[5];
 	char		out[16 + 1];
-	char		fbuff[] = "                        xyzzy";
+	char		fbuff[24];
+	const char	PATTERN[] = "xyzzy";
+#define PATTERN_LEN (sizeof(PATTERN) - 1)
 	char		*post_ws;
 	ssize_t		slen;
 
+	static_assert(sizeof(buff) >= PATTERN_LEN, "Buffer must be sufficiently large to hold the pattern");
+	static_assert((sizeof(fbuff) % sizeof(buff)) > 0, "sizeof buff must not be a multiple of fbuff");
+	static_assert((sizeof(fbuff) % sizeof(buff)) < PATTERN_LEN, "remainder of sizeof(fbuff)/sizeof(buff) must be less than sizeof pattern");
+
 	TEST_CASE("Initialization");
-	fp = fmemopen(fbuff, sizeof(fbuff) - 1, "r");
+	memset(fbuff, ' ', sizeof(fbuff));
+	memcpy(fbuff + sizeof(fbuff) - PATTERN_LEN, PATTERN, PATTERN_LEN);
+
+	fp = fmemopen(fbuff, sizeof(fbuff), "r");
 	TEST_CHECK(fp != NULL);
 	TEST_CHECK(fr_sbuff_init_file(&sbuff, &fctx, buff, sizeof(buff), fp, 128) == &sbuff);
 	our_sbuff = FR_SBUFF_BIND_CURRENT(&sbuff);
 
 	TEST_CASE("Advance past whitespace, which will require shift/extend");
-	TEST_CHECK_LEN(fr_sbuff_adv_past_whitespace(&our_sbuff, SIZE_MAX, NULL), sizeof(fbuff) - 6);
+	TEST_CHECK_LEN(fr_sbuff_adv_past_whitespace(&our_sbuff, SIZE_MAX, NULL), sizeof(fbuff) - PATTERN_LEN);
+	TEST_CASE("Verify extend on unused child buffer");
+	child_sbuff = FR_SBUFF(&our_sbuff);
+	slen = fr_sbuff_extend_file(&child_sbuff, 0);
+	TEST_CHECK_SLEN(slen, sizeof(fbuff) % PATTERN_LEN);
 	TEST_CASE("Verify that we passed all and only whitespace");
 	(void) fr_sbuff_out_abstrncpy(NULL, &post_ws, &our_sbuff, 24);
-	TEST_CHECK_STRCMP(post_ws, "xyzzy");
+	TEST_CHECK_STRCMP(post_ws, PATTERN);
 	talloc_free(post_ws);
 	TEST_CASE("Verify parent buffer end");
 	TEST_CHECK(sbuff.end == our_sbuff.end);