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.

GitHub Relationships

• Resolves qprintf() prints % and & inside skipped conditionals #140.

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?

[gbeeley]

A few issues, some are points for discussion, and some for further investigation. Thanks!

[gbeeley]

limit is a hard limit on the maximum size of the insertion. This is independent of the ability to grow the destination: if nLEN / *LEN is used in the spec chain, that places a hard limit on the amount of data that can be inserted for the one specific conversion/translation. The destination may be able to be grown larger (or may otherwise have plenty of space already), but that's a separate issue.

Besides fixing this function doc, review the refactored code to ensure it is consistent with this use of limit.

[Lightning11wins]

I was a little confused by this but now understand much better and I've refined the doc comments.

[Lightning11wins]

I've just conducted a review and I believe my refactored code implements this feature correctly.

[gbeeley]

This appears to ignore filters that aren't found / aren't handled. This at least deserves a warning message (yes, this was from the original code, not from this PR). However, the commit history indicates this was a deliberate choice due to   occurring in the format string. It seems that only happens in a few places and could easily be worked around via %&. Not sure if there are other implications.

[Lightning11wins]

After looking into this, it looks like you were right. &nbsp directly after a specifier will trigger the warning but %&nbsp does not.

According to Claude, this contract change only affects two places in production (both in htdrv_tab.c). I've updated those on this branch so that the merge conflict on apos_autoscale9-slim5 will remind me to scan it for this issue, although I don't expect to find much.

[Lightning11wins]

An unexpected side effect is that this warning caused the tests to fail by lockup because printing this warning hundreds of thousands of times slows down one of the tests a lot.

This doesn't seem like a real bug detected from the test (calling qprintf() with a format that triggers this warning hundreds of thousands of times per second doesn't seem like a usecase we need to support), so I changed the test to pipe stderr to /dev/null, fixing the lockup.

[gbeeley]

Is there a reason for allowing consecutive & characters without filters?

[Lightning11wins]

I don't think I do? An invalid specifier breaks out of the do {} while() loop (see above). Thus, additional &s after a specifier are not consumed. I think this is the same behavior as master.

For example, the format specifier x%STR&&y& (with "a<b" passed) results in: xa<b&&y&.

[gbeeley]

This should be a #define somewhere.

[Lightning11wins]

I couldn't find a pre-existing define for this so I defined it above. We should probably have a lint rule against magic numbers in code.

[Lightning11wins]

I added a rule against magic numbers to the style guide.

[gbeeley]

Hopefully snprintf() doesn't fail here. Good to check. (similar in other nearby lines)

[Lightning11wins]

Added error handling.

[gbeeley]

Let's avoid goto in this usage (not an error/exit/retry situation, not a state machine).

[Lightning11wins]

Oh, great. This refactor is about to wreck the git blame.

[Lightning11wins]

I've refactored to use an if statement instead and I think the resulting code is better, thanks for the feedback.

[gbeeley]

Setting min_room and quote here works in the current implementation where we don't allow the chaining of translation/filter specs yet. But they probably should be moved into the spec loop so this doesn't become a latent bug if chaining of multiple filters is implemented in the future.

[Lightning11wins]

On master, these values are defined in the function's top-level scope. I didn't understand them super well so I moved them to the lowest scope I felt I safely could. Thanks for the clarification, I've fixed this.

[gbeeley]

There used to be a check here that verified that n_chars returned from Translate matched the change in the dest_offset. The check appears to be aimed at preventing multiple successive failed grow function calls if a grow already failed, as in other areas of the code.

[Lightning11wins]

Ahh, looks like this regression came out of an incorrect fix I made in the past that weakened this check. Fixed.

[gbeeley]

This space needed / error logic appears to be different from the original and different from the other plain string copying code early in this function. Do a double check review for correctness on this. The switch from a computed-each-time space needed expression to a computed-once space needed expression could create some divergence in the logic from the original.

[Lightning11wins]

I don't fully understand this logic, but I think I fixed it.

[Lightning11wins]

We should possibly consider making the repeated grow logic a helper function. Despite my best effort to make it easy to read, it's one of the most confusing parts of the refactored code.

[Lightning11wins]

@gbeeley I believe I've addressed all the concerns from your review.

[Lightning11wins]

I've fixed several other bugs that I found on my own, now, too. I think that's everything, though.