Skip to content

Replace the 'p += snprintf' idiom with a bounded append - #592

Closed
somethingwithproof wants to merge 1 commit into
Cacti:developfrom
somethingwithproof:fix/bounded-append
Closed

Replace the 'p += snprintf' idiom with a bounded append#592
somethingwithproof wants to merge 1 commit into
Cacti:developfrom
somethingwithproof:fix/bounded-append

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Closes #588.

remaining = SIZE - (p - buf);
p += snprintf(p, remaining, "...", ...);

snprintf returns the length it would have written, so the first truncation moves the cursor past the end of the buffer. The next remaining is then negative, and as a size_t it is effectively unbounded, at a destination that is already out of bounds.

The idiom appears at 53 sites, 47 in util.c and 6 in spine.c.

The one that is already wrong

util.c:829-851 passed a fixed BUFSIZE to every call rather than a shrinking remainder, so the bound never moved at all:

sqlp += snprintf(sqlp, BUFSIZE, "SELECT SQL_NO_CACHE action FROM poller_item");
sqlp += snprintf(sqlp, BUFSIZE, " WHERE action=%d", POLLER_ACTION_PHP_SCRIPT_SERVER);
sqlp += snprintf(sqlp, BUFSIZE, " AND host_id IN(%s)", set.host_id_list);

--hostlist can fill set.host_id_list with up to 65534 bytes. The third call writes at most 1023 of them, then advances sqlp by about 65550, and the query that reaches db_query() is the truncated prefix. The buffer is HUGE_BUFSIZE, so this is malformed SQL rather than a stack overflow, but nothing here is doing what it reads as doing.

poller_push_data_to_main() is where the margin matters: on the stock schema it lands near 1.75 MB against a 2.048 MB buffer, about 15 percent, with a guard that does not work.

The change

spine_appendf() owns the arithmetic, advances by what was actually written, and returns FALSE on truncation. On truncation it leaves the cursor on the terminator, so the buffer stays a valid string and every later append fails there rather than running past the end.

All 53 sites are converted. Each recompute line is kept:

remaining = HUGE_BUFSIZE - (sqlp - sqlbuf);
spine_appendf(&sqlp, &remaining, "%s, ", row[0]);

spine_appendf() maintains remaining itself, so the recompute is redundant. Dropping it would mean finding the right initialisation point for every cursor in a function that resets sqlp inside loops. Keeping it makes the conversion purely local: no buffer depends on being initialised somewhere new, and the recompute still self-heals. Removing them is a separate, later change.

Two int remaining declarations became size_t.

Tests

Seven cases against the shipped util.o, each using a struct with a 0x7e canary after the buffer so an overflow is observable rather than merely undefined:

  • appends and advances by what was written
  • successive appends accumulate
  • truncation returns FALSE, leaves the cursor inside the buffer on the terminator, and sets remaining to 1
  • a second append after truncation writes nothing, anywhere
  • NULL cursor, NULL target and NULL remainder are refused
  • an exhausted buffer is refused without touching it
  • test_old_idiom_overshoots_where_appendf_does_not runs the old expression and asserts the cursor lands outside the buffer and the next remainder is negative, then the same input through spine_appendf() and asserts it does not

That last one is the point: it fails if the defect is ever not a defect, and it documents why the other six exist.

44 of 44 pass. Clean ubuntu:24.04 rebuild carries the same 4 warnings as develop.

No CHANGELOG entry here; #578 is the changelog PR for this batch.

snprintf returns the length it would have written, so the cursor moves past
the buffer on the first truncation and the next remainder underflows to a
huge size_t: an effectively unbounded size at a destination already out of
bounds.

util.c:829-851 was the worst of it. Every call there passed a fixed BUFSIZE
rather than a shrinking remainder, so the bound never moved, and one of them
interpolates set.host_id_list, which --hostlist can fill with 65534 bytes.

spine_appendf() owns the arithmetic and reports truncation. Converted all 53
sites, keeping each recompute line so no buffer depends on being initialised
in a new place.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof

Copy link
Copy Markdown
Member Author

Consolidated into #597, which carries this branch's commits unchanged.

Every pair of these ten conflicted on tests/unit/test_linked.c because each appended to the same registration array, so whichever merged first would have forced a rebase on the other nine. #597 is one review and one approval for the same 20 commits, still one logical change each.

Nothing here is dropped. Reopen this if you would rather review it separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The 'p += snprintf' idiom moves the pointer past the buffer on truncation

1 participant