Have fr_sbuff_extend_lowat check the eof state of the sbuff. Fixes #5462

Don't extend the sbuff in the fr_sbuff_terminal_search function

src/lib/util/sbuff.c

@@ -262,7 +262,7 @@ size_t fr_sbuff_shift(fr_sbuff_t *sbuff, size_t shift)
 /** Refresh the buffer with more data from the file
  *
  */
-size_t fr_sbuff_extend_file(fr_sbuff_t *sbuff, size_t extension)
+size_t fr_sbuff_extend_file(fr_sbuff_extend_status_t *status, fr_sbuff_t *sbuff, size_t extension)
 {
 	fr_sbuff_t		*sbuff_i;
 	size_t			read, available, total_read, shift;
@@ -323,6 +323,7 @@ size_t fr_sbuff_extend_file(fr_sbuff_t *sbuff, size_t extension)
 	if (read < available) {
 		if (!feof(fctx->file)) {	/* It's a real error */
 			fr_strerror_printf("Error extending buffer: %s", fr_syserror(ferror(fctx->file)));
+			*status |= FR_SBUFF_FLAG_EXTEND_ERROR;
 			return 0;
 		}
 
@@ -332,16 +333,26 @@ size_t fr_sbuff_extend_file(fr_sbuff_t *sbuff, size_t extension)
 	return read;
 }
 
+/** Accessor function for the EOF state of the file extendor
+ *
+ */
+bool fr_sbuff_eof_file(fr_sbuff_t *sbuff)
+{
+	fr_sbuff_uctx_file_t	*fctx = sbuff->uctx;
+	return fctx->eof;
+}
+
 /** Reallocate the current buffer
  *
+ * @param[in] status		Extend status.
  * @param[in] sbuff		to be extended.
  * @param[in] extension		How many additional bytes should be allocated
  *				in the buffer.
  * @return
  *	- 0 the extension operation failed.
  *	- >0 the number of bytes the buffer was extended by.
  */
-size_t fr_sbuff_extend_talloc(fr_sbuff_t *sbuff, size_t extension)
+size_t fr_sbuff_extend_talloc(fr_sbuff_extend_status_t *status, fr_sbuff_t *sbuff, size_t extension)
 {
 	fr_sbuff_uctx_talloc_t	*tctx = sbuff->uctx;
 	size_t			clen, nlen, elen = extension;
@@ -386,6 +397,7 @@ size_t fr_sbuff_extend_talloc(fr_sbuff_t *sbuff, size_t extension)
 	new_buff = talloc_realloc(tctx->ctx, sbuff->buff, char, nlen);
 	if (unlikely(!new_buff)) {
 		fr_strerror_printf("Failed extending buffer by %zu bytes to %zu bytes", elen, nlen);
+		*status |= FR_SBUFF_FLAG_EXTEND_ERROR;
 		return 0;
 	}
 
@@ -536,8 +548,6 @@ static inline bool fr_sbuff_terminal_search(fr_sbuff_t *in, char const *p,
 	ssize_t		mid;
 
 	size_t		remaining;
-	bool		reset_p = (p == in->p);
-	fr_sbuff_extend_status_t	status = FR_SBUFF_EXTENDABLE;
 
 	if (!term) return false;			/* If there's no terminals, we don't need to search */
 
@@ -549,13 +559,22 @@ static inline bool fr_sbuff_terminal_search(fr_sbuff_t *in, char const *p,
 	/*
 	 *	Special case for EOFlike states
 	 */
-	remaining = fr_sbuff_extend_lowat(&status, in, needle_len);
-	if (remaining == 0) {
-		if (status & FR_SBUFF_EXTEND_ERROR) return false;
-		return (idx['\0'] != 0);
+	remaining = fr_sbuff_remaining(in);
+	if ((remaining == 0) && !fr_sbuff_is_extendable(in)) {
+		if (idx['\0'] != 0) return true;
+		return false;
 	}
 
-	if (reset_p) p = in->p;
+	if (remaining < needle_len) {
+		fr_assert_msg(!fr_sbuff_is_extendable(in),
+			      "Caller failed to extend buffer by %zu bytes before calling fr_sbuff_terminal_search",
+			      needle_len);
+		/*
+		 *	We can't search for the needle if we don't have
+		 *	enough data to match it.
+		 */
+		return false;
+	}
 
 	mid = term_idx - 1;				/* Inform the mid point from the index */
 
@@ -925,8 +944,7 @@ size_t fr_sbuff_out_unescape_until(fr_sbuff_t *out, fr_sbuff_t *in, size_t len,
 
 	uint8_t				idx[UINT8_MAX + 1];			/* Fast path index */
 	size_t				needle_len = 1;
-
-	fr_sbuff_extend_status_t	status = FR_SBUFF_EXTENDABLE;		/* Tracks if we can extend */
+	fr_sbuff_extend_status_t	status = 0;
 
 	/*
 	 *	If we don't need to do unescaping
@@ -2140,10 +2158,10 @@ bool fr_sbuff_is_terminal(fr_sbuff_t *in, fr_sbuff_term_t const *tt)
 	 *	No terminal, check for EOF.
 	 */
 	if (!tt) {
-		fr_sbuff_extend_status_t status = FR_SBUFF_EXTENDABLE;
+		fr_sbuff_extend_status_t status = 0;
 
 		if ((fr_sbuff_extend_lowat(&status, in, 1) == 0) &&
-		    (status & FR_SBUFF_EXTEND_ERROR) == 0) {
+		    (status & FR_SBUFF_FLAG_EXTEND_ERROR) == 0) {
 			return true;
 		}
 
@@ -2156,6 +2174,8 @@ bool fr_sbuff_is_terminal(fr_sbuff_t *in, fr_sbuff_term_t const *tt)
 	 */
 	fr_sbuff_terminal_idx_init(&needle_len, idx, tt);
 
+	fr_sbuff_extend_lowat(NULL, in, needle_len);
+
 	return fr_sbuff_terminal_search(in, in->p, idx, tt, needle_len);
 }
 

src/lib/util/sbuff.h

@@ -51,13 +51,36 @@ extern "C" {
 typedef ssize_t fr_slen_t;
 typedef struct fr_sbuff_s fr_sbuff_t;
 typedef struct fr_sbuff_ptr_s fr_sbuff_marker_t;
-typedef size_t(*fr_sbuff_extend_t)(fr_sbuff_t *sbuff, size_t req_extension);
 
 #include <freeradius-devel/util/atexit.h>
 #include <freeradius-devel/util/strerror.h>
 #include <freeradius-devel/util/table.h>
 #include <freeradius-devel/util/talloc.h>
 
+/** Whether the buffer is currently extendable and whether it was extended
+ *
+ */
+DIAG_OFF(attributes)
+typedef enum CC_HINT(flag_enum) {
+	FR_SBUFF_FLAG_EXTENDED			= 0x01,		//!< The last call to extend function actually extended the buffer.
+	FR_SBUFF_FLAG_EXTEND_ERROR		= 0x02		//!< The last call to an extend function resulted in an error.
+								///< Error should be provided using fr_strerror_const/fr_strerror_printf
+								///< by the extension function.
+} fr_sbuff_extend_status_t;
+DIAG_OFF(attributes)
+
+/** Extension callback
+ *
+ * Retrieves additional data from a source and adds it to a buffer.
+ */
+typedef size_t(*fr_sbuff_extend_t)(fr_sbuff_extend_status_t *status, fr_sbuff_t *sbuff, size_t req_extension);
+
+/** For a given extension function, returns whether it is at EOF
+ *
+ */
+typedef bool(*fr_sbuff_eof_t)(fr_sbuff_t *sbuff);
+
+
 struct fr_sbuff_ptr_s {
 	union {
 		char const *p_i;				//!< Immutable position pointer.
@@ -98,6 +121,8 @@ struct fr_sbuff_s {
 	fr_sbuff_extend_t		extend;			//!< Function to re-populate or extend
 								///< the buffer.
 
+	fr_sbuff_eof_t			eof;			//!< Function to determine if the buffer is at EOF.
+
 	void				*uctx;			//!< Extend uctx data.
 
 	fr_sbuff_t			*parent;		//!< sbuff this sbuff was copied from.
@@ -246,23 +271,14 @@ static inline void fr_sbuff_parse_error_to_strerror(fr_sbuff_parse_error_t err)
 	fr_strerror_const(fr_table_str_by_value(sbuff_parse_error_table, err, "<INVALID>"));
 }
 
-#define FR_SBUFF_FLAG_EXTENDABLE		0x01
-#define FR_SBUFF_FLAG_EXTENDED			0x02
-#define FR_SBUFF_FLAG_EXTEND_ERROR		0x04
-
-/** Whether the buffer is currently extendable and whether it was extended
- *
+/** Return whether the sbuff is extendable
  */
-typedef enum {
-	FR_SBUFF_NOT_EXTENDABLE			= 0x00,
-	FR_SBUFF_EXTENDABLE			= FR_SBUFF_FLAG_EXTENDABLE,
-	FR_SBUFF_EXTENDABLE_EXTENDED		= FR_SBUFF_FLAG_EXTENDABLE | FR_SBUFF_FLAG_EXTENDED,
-	FR_SBUFF_EXTENDED			= FR_SBUFF_FLAG_EXTENDED,
-	FR_SBUFF_EXTEND_ERROR			= FR_SBUFF_FLAG_EXTEND_ERROR
-} fr_sbuff_extend_status_t;
+static inline bool fr_sbuff_is_extendable(fr_sbuff_t *sbuff)
+{
+	return sbuff->extend && (!sbuff->eof || (sbuff->eof(sbuff) == false));
+}
 
-#define fr_sbuff_is_extendable(_status)		((_status) & FR_SBUFF_FLAG_EXTENDABLE)
-#define fr_sbuff_was_extended(_status)		((_status) & FR_SBUFF_FLAG_EXTENDED)
+#define fr_sbuff_was_extended(_status)		(_status & FR_SBUFF_FLAG_EXTENDED)
 
 extern bool const sbuff_char_class_uint[UINT8_MAX + 1];
 extern bool const sbuff_char_class_int[UINT8_MAX + 1];
@@ -401,7 +417,7 @@ do { \
  *
  * @private
  */
-#define _FR_SBUFF(_sbuff_or_marker, _start, _current, _end, _extend, _adv_parent) \
+#define _FR_SBUFF(_sbuff_or_marker, _start, _current, _end, _extend, _eof, _adv_parent) \
 ((fr_sbuff_t){ \
 	.buff		= fr_sbuff_buff(_sbuff_or_marker), \
 	.start		= (_start), \
@@ -411,6 +427,7 @@ do { \
 	.adv_parent 	= (_adv_parent), \
 	.shifted	= 0, \
 	.extend		= (_extend), \
+	.eof		= (_eof), \
 	.uctx		= fr_sbuff_ptr(_sbuff_or_marker)->uctx, \
 	.parent 	= fr_sbuff_ptr(_sbuff_or_marker) \
 })
@@ -428,6 +445,7 @@ do { \
 					     fr_sbuff_current(_sbuff_or_marker), \
 					     fr_sbuff_end(_sbuff_or_marker), \
 					     fr_sbuff_ptr(_sbuff_or_marker)->extend, \
+					     fr_sbuff_ptr(_sbuff_or_marker)->eof, \
 					     0x00)
 
 /** Create a new sbuff pointing to the same underlying buffer
@@ -461,6 +479,7 @@ do { \
 						     fr_sbuff_start(_sbuff_or_marker), \
 						     fr_sbuff_current(_sbuff_or_marker), \
 						     NULL, \
+						     NULL, \
 						     0x00)
 
 /** Create a new sbuff pointing to the same underlying buffer
@@ -475,6 +494,7 @@ do { \
 							  fr_sbuff_current(_sbuff_or_marker), \
 							  fr_sbuff_end(_sbuff_or_marker), \
 							  fr_sbuff_ptr(_sbuff_or_marker)->extend, \
+							  fr_sbuff_ptr(_sbuff_or_marker)->eof, \
 							  0x01)
 
 /** Create a new sbuff pointing to the same underlying buffer
@@ -489,6 +509,7 @@ do { \
 								 fr_sbuff_current(_sbuff_or_marker), \
 								 fr_sbuff_end(_sbuff_or_marker), \
 								 fr_sbuff_ptr(_sbuff_or_marker)->extend, \
+								 fr_sbuff_ptr(_sbuff_or_marker)->eof, \
 								 0x01)
 
 /** Creates a compound literal to pass into functions which accept a sbuff
@@ -576,9 +597,11 @@ void	fr_sbuff_update(fr_sbuff_t *sbuff, char *new_buff, size_t new_len);
 
 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);
+size_t	fr_sbuff_extend_file(fr_sbuff_extend_status_t *status, fr_sbuff_t *sbuff, size_t extension);
+
+bool	fr_sbuff_eof_file(fr_sbuff_t *sbuff);
 
-size_t	fr_sbuff_extend_talloc(fr_sbuff_t *sbuff, size_t extension);
+size_t	fr_sbuff_extend_talloc(fr_sbuff_extend_status_t *status, fr_sbuff_t *sbuff, size_t extension);
 
 int	fr_sbuff_trim_talloc(fr_sbuff_t *sbuff, size_t len);
 
@@ -681,6 +704,7 @@ static inline fr_sbuff_t *fr_sbuff_init_file(fr_sbuff_t *sbuff, fr_sbuff_uctx_fi
 		.p = buff,
 		.end = buff,			//!< Starts with 0 bytes available
 		.extend = fr_sbuff_extend_file,
+		.eof = fr_sbuff_eof_file,
 		.uctx = fctx
 	};
 
@@ -989,26 +1013,27 @@ static inline fr_slen_t _fr_sbuff_error(fr_sbuff_t *sbuff, char const *err)
 #define FR_SBUFF_CHECK_REMAINING_RETURN(_sbuff, _len) \
 	if ((_len) > fr_sbuff_remaining(_sbuff)) return -((_len) - fr_sbuff_remaining(_sbuff))
 
-static inline size_t _fr_sbuff_extend_lowat(fr_sbuff_extend_status_t *status, fr_sbuff_t *in,
-					    size_t remaining, size_t lowat)
+static inline size_t _fr_sbuff_extend_lowat(fr_sbuff_extend_status_t *status, fr_sbuff_t *in, size_t remaining, size_t lowat)
 {
 	size_t extended;
+	fr_sbuff_extend_status_t our_status = 0;
 
-	if (status && !(*status & FR_SBUFF_EXTENDABLE)) {
-	not_extendable:
-		if (status) *status = FR_SBUFF_NOT_EXTENDABLE;
+	if (!fr_sbuff_is_extendable(in)) {
+	no_extend:
+		if (status) *status = our_status;
 		return remaining;
 	}
 
-	if (remaining >= lowat) {
-		if (status) *status = FR_SBUFF_EXTENDABLE;
-		return remaining;
-	}
+	/* Still have data remaining, no need to try and extend */
+	if (remaining >= lowat) goto no_extend;
 
-	if (!in->extend || !(extended = in->extend(in, lowat - remaining))) goto not_extendable;
+	if (!in->extend || ((extended = in->extend(&our_status, in, lowat - remaining)) == 0)) {
+		goto no_extend;
+	}
 
-	if (status) *status = FR_SBUFF_EXTENDABLE_EXTENDED;
+	our_status |= FR_SBUFF_FLAG_EXTENDED;
 
+	if (status) *status = our_status;
 	return remaining + extended;
 }
 

src/lib/util/sbuff_tests.c

@@ -1068,7 +1068,7 @@ static void test_file_extend(void)
 	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);
+	slen = fr_sbuff_extend_file(NULL, &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);