Qprintf Rewrite

[Lightning11wins]

qprintf.c had some code that confused me a lot. For example, there was a switch statement inside an if statement inside another switch statement inside a loop inside another if statement inside the else block of different if statement inside another if statement inside another loop... causing my brain to throw a stack overflow exception.

After understanding the code (which will be useful for the apos_autoscale proejct), I did some refactoring, and now it's much easier to read, in my opinion. Also, I added some tools for improved error handling, as usual.

Other changes (not in this PR):
I also wrote additional tests for qprintf, which passed, but they require several other PRs as dependencies, so I'll add them to the centrallix-lib-tests branch once I update it after the dups PR is merged.

[Lightning11wins]

@greptileai Please review this PR.

[greptile-apps]

Greptile Summary

This PR refactors qprintf.c for readability by flattening the deeply-nested original logic, renaming variables to be more descriptive, and splitting the do-while loop into a while(1) with explicit break. It also adds new public API functions (qpfPrintf_g, qpfPrintf_gva, qpfNoGrow, qpfSysMallocGrow, qpfLogErrors) and expands the QPSession struct to track per-error line numbers via a new ErrorLines array.

• The new QPERR macro replaces the one-liner (s->Errors |= x) with a GCC statement-expression block that records the source line of each error into ErrorLines, protected by a qpf_internal_count_zeros helper and an assert guard.
• Several previously-reported bugs in this review cycle (missing no_grow guards on grow-fn calls, closing-quote buffer underflow, null_session.ErrorLines initialization) have been addressed, and two new regression tests (test_qprintf_69, test_qprintf_70) exercise the size-2 and size-1 quote-overflow paths.

Confidence Score: 5/5

The refactor is behaviorally equivalent to the original and the previously-flagged bugs have been addressed; only minor documentation and portability observations remain.

The core formatting engine logic has been carefully preserved across the refactor. All previously-identified overflow and guard bugs have been fixed and are covered by the two new regression tests. The remaining observations are documentation clarity and a non-standard macro extension the project already implicitly depends on elsewhere.

No files require special attention beyond the inline documentation notes in qprintf.c.

Important Files Changed

Filename	Overview
centrallix-lib/src/qprintf.c	Major refactor of the core formatting engine; previously-flagged bugs are fixed; QPERR macro uses GCC statement expressions; a duplicate @param entry and an uncertain comment were left in the docstrings
centrallix-lib/include/qprintf.h	Public API extended with new grow helpers and qpfLogErrors; error constants moved from decimal to bit-shift style and QPSession gains ErrorLines array
centrallix-lib/tests/test_qprintf_69.c	New test: verifies %STR&QUOT with a 2-byte buffer writes opening quote + null terminator and returns full logical count (4)
centrallix-lib/tests/test_qprintf_70.c	New test: verifies the size-1 buffer underflow bug is fixed — %STR&QUOT with a 1-byte buffer writes only a null terminator and does not clobber adjacent memory
centrallix-sysdoc/QPrintf.md	Documentation updated with new API functions, improved specifier descriptions, and formatting improvements

_{Reviews (12): Last reviewed commit: "Fix compiler error." | Re-trigger Greptile}

[Lightning11wins]

@greptileai I think I resolved your comments. Please rereview. Are there any other concerns you have?

[Lightning11wins]

@greptileai I think I resolved your comments. Please rereview. Are there any other concerns you have?

[greptile-apps]

The root cause is that min_room = 2 only establishes the 2-byte reserve after Translate writes something. When no_grow is already true on entry, Translate's hard route short-circuits and writes nothing — dest_offset is unchanged, so the reserve is never created.

Instead of making the write conditional (which breaks your tests), clamp dest_offset right before the unconditional write:

			    no_grow = true;
			    }
			
			/** Write the closing quote, even if a buffer overflow occurred. **/
			/** We passed min_room = 2, so there will be enough space for it. **/
			/** However, if Translate wrote nothing (e.g. no_grow was already  **/
			/** true on entry), the reserve may not have been established — so  **/
			/** clamp dest_offset to guarantee room for quote + null terminator. **/
			if (dest_offset + 2 > *dest_size)
			    dest_offset = *dest_size - 2;
			(*dest)[dest_offset++] = quote;

The clamp is a no-op in the normal case — when Translate respected min_room = 2, dest_offset + 2 <= *dest_size already holds. It only activates when the buffer was already full on entry to the Translate call, ensuring the closing quote and null terminator always fit. This preserves the contract test_qprintf_51 and test_qprintf_52 rely on.

[Lightning11wins]

@greptileai I've made the changes you've requested but I haven't tested or pushed my code yet. Could you write a test case, following the same style established in test_qprintf_51.c that will ensure that will verify that this issue is fixed? Thus, it should fail on the previous implementation (the one that did not move the pointer AND the one that did not check no_grow), but it should pass on the current implementation that you showed me, which handles the situation correctly.

[greptile-apps]

Here's the test following the exact style of test_qprintf_51.c/test_qprintf_52.c.

Why this reproduces the bug: A 2-byte buffer for %STR&QUOT with input "ab" forces the exact path in question.

1. Opening quote written at buf[4], dest_offset = 1.
2. Translate writes nothing (no_grow triggered — no room for content + min_room = 2).
3. Closing-quote path: dest_offset + 2 = 3 > 2.
   • Without dest_offset clamp: closing quote written at buf[5], dest_offset = 2. Null-term guard 2 > 2 is false → no null written. buf[5] == '\''.
   • Without no_grow guard: grow_fn is called on a static buffer, returns false, falls through to same unconditional write → same result.
   • With the full fix: dest_offset clamped to *dest_size - 2 = 0, closing quote at buf[4], null at buf[5]. ✓

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <string.h>
#include <stdlib.h>
#include "qprintf.h"
#include <assert.h>

long long
test(char** tname)
    {
    int i, rval;
    int iter;
    unsigned char buf[16];

	*tname = "qprintf-53 %STR&QUOT with buffer too small for content (null termination)";
	iter = 200000;
	for(i=0;i<iter;i++)
	    {
	    buf[9]  = '\n';
	    buf[8]  = '\0';
	    buf[7]  = 0xff;
	    buf[6]  = 0xff;  /* sentinel: byte immediately after 2-byte buffer -- must not be clobbered */
	    /* buf+4 is the 2-byte buffer under test: occupies buf[4] and buf[5] */
	    buf[3]  = '\n';
	    buf[2]  = '\0';
	    buf[1]  = 0xff;
	    buf[0]  = '\0';
	    rval = qpfPrintf(NULL, buf+4, 2, "%STR&QUOT", "ab");
	    /* The 2-byte buffer must be null-terminated within its own bounds.
	     * Without dest_offset clamped before the closing-quote write,
	     * buf[5] holds '\'' and no null terminator is ever written. */
	    assert(buf[5] == '\0');
	    /* The byte immediately after the buffer must not be overwritten.
	     * Without the no_grow guard, an out-of-bounds null-terminator write
	     * could clobber this sentinel. */
	    assert(buf[6] == 0xff);
	    /* Bytes before the buffer must be untouched */
	    assert(buf[3] == '\n');
	    assert(buf[2] == '\0');
	    assert(buf[1] == 0xff);
	    assert(buf[0] == '\0');
	    /* Return value must indicate overflow: full output 'ab' + NUL = 5 bytes */
	    assert(rval == 5);
	    }

    return iter;
    }

Failure summary by variant:

Variant	buf[5] after call	assert(buf[5] == '\0')
No no_grow guard (original)	'\'' — closing quote, no null	FAILS
Guard added, no dest_offset clamp	'\'' — same	FAILS
Full fix (guard + clamp)	'\0'	passes

assert(buf[6] == 0xff) is a secondary tripwire: if a null terminator were written one byte past the buffer instead of being omitted, that sentinel catches it regardless.

One caveat: I inferred rval == 5 from the pattern in tests 51/52 (return value = bytes needed including NUL, for 'ab' that's 5). If the actual semantics differ, substitute assert(rval > 2).

[Lightning11wins]

@greptileai I think I resolved your comments, and I also added your test case with some clean up, bug fixing (on the return value), and other improvements. Please rereview. Are there any other concerns you have?

[Lightning11wins]

@greptileai I think I resolved your comments. Please rereview. Are there any other concerns you have?

[Lightning11wins]

@greptileai I think I resolved your comments, and I also added your test case with some clean up, bug fixing (on the return value), and other improvements. Please rereview. Are there any other concerns you have?

[Lightning11wins]

@greptileai I just put some finishing touches on the project. Could you review this please?

[Lightning11wins]

This PR is cleared for human review.

[Lightning11wins]

@greptileai Please re-review the new changes in commit 1a0faf8 for this PR.

[Lightning11wins]

This PR is once again cleared for human review.

[Lightning11wins]

This PR is once again cleared for human review.