Signed-ness problems with NANOPRINTF_CONVERSION_BUFFER_SIZE

[stefano-zanotti-88]

This line:

nanoprintf/nanoprintf.h

Line 558 in caafd3a

if (spec->prec > (NANOPRINTF_CONVERSION_BUFFER_SIZE - 2)) { goto exit; }

misbehaves if NANOPRINTF_CONVERSION_BUFFER_SIZE is unsigned.
This may actually happen, as the symbol can be provided by the user. In this case, the failure would be very hard to spot and even to understand, for the user. Also, it's not uncommon for the user to use an unsigned literal, since this symbol determines a buffer size.
The problem arises because in case of a signed-unsigned operation, the signed one gets casted to unsigned (after possible promotions).

Similarly in these places:

nanoprintf/nanoprintf.h

Line 510 in caafd3a

#if (NANOPRINTF_CONVERSION_BUFFER_SIZE <= UINT_FAST8_MAX) && (UINT_FAST8_MAX <= INT_MAX)

nanoprintf/nanoprintf.h

Line 595 in caafd3a

if (dec >= NANOPRINTF_CONVERSION_BUFFER_SIZE) { goto exit; }

nanoprintf/nanoprintf.h

Line 607 in caafd3a

if (end >= NANOPRINTF_CONVERSION_BUFFER_SIZE) { goto exit; }

nanoprintf/nanoprintf.h

Line 677 in caafd3a

if (dec >= NANOPRINTF_CONVERSION_BUFFER_SIZE) { goto exit; }

Edit: the problem does not exist if dec and end are never negative, regardless of their being signed. The same for prec above; but, are we sure that it is never negative? Not even when it was indeed negative and it was ignored by setting prec_opt = NPF_FMT_SPEC_OPT_NONE but without setting prec = 0 ?

This is a tricky topic at least for me; maybe the problem is not real at all? However, I think it can arise in some cases and not in others -- ie it might depend on the relative bit-size of NANOPRINTF_CONVERSION_BUFFER_SIZE and the other variables involved in math operations with it.
For instance:
-1 > 23u - 2 is true [unexpected] (-1 > 21u) -> (0xFFFFFFFFu > 21u)
-1 > 23ull - 2 is true [unexpected] (-1 > 21ull) -> (0xFFFFFFFFFFFFFFFFu > 21ull)
-1 > (unsigned short)23 - 2 is false [expected] (-1 > (int)(unsigned short)23 - 2) -> (-1 > 23 - 2) -> (-1 > 21)

I think a cast-to-signed of NANOPRINTF_CONVERSION_BUFFER_SIZE is needed everywhere, possibly with a type tailored to every occurrence. Or maybe a cast to (int) is always acceptable, as long as we change the check here:

nanoprintf/nanoprintf.h

Line 72 in caafd3a

#if NANOPRINTF_CONVERSION_BUFFER_SIZE < 23

by adding

&& NANOPRINTF_CONVERSION_BUFFER_SIZE < UINT_MAX / 2

which ensures that it already fits in an (int), so the cast to (int) just removes the unsigned-ness.

Note that the check on line 510 is not enough to prevent the problem, as can be demonstrated by the 23ull example above.

Maybe the code can be kept the same, and we just need the following check. We already know from this:

nanoprintf/nanoprintf.h

Line 72 in caafd3a

#if NANOPRINTF_CONVERSION_BUFFER_SIZE < 23

that NANOPRINTF_CONVERSION_BUFFER_SIZE is positive.
So, this check should be enough:

#if (-1 < (NANOPRINTF_CONVERSION_BUFFER_SIZE) && -1l < (NANOPRINTF_CONVERSION_BUFFER_SIZE) && -1ll < (NANOPRINTF_CONVERSION_BUFFER_SIZE))
  // ok, it's a signed literal
#else
  #error NANOPRINTF_CONVERSION_BUFFER_SIZE can't be an unsigned literal
#endif

Finally: this symbol is provided by the user, so every usage of it should be parenthesized. Especially since the user might compute it from other values they have.
This is not the case for NANOPRINTF_CONVERSION_FLOAT_TYPE, because it is supposed to be a type, and parentheses would not be parsed; also, it is much less likely to be the result of an expression, than it is for a numeric option like the buffer size.

[stefano-zanotti-88]

It seems that the only case in which prec can ever be negative is this:

nanoprintf/nanoprintf.h

Lines 783 to 788 in caafd3a

	#if NANOPRINTF_USE_PRECISION_FORMAT_SPECIFIERS == 1
	if (fs.prec_opt == NPF_FMT_SPEC_OPT_STAR) {
	fs.prec = va_arg(args, int);
	if (fs.prec < 0) { fs.prec_opt = NPF_FMT_SPEC_OPT_NONE; }
	}
	#endif

For which is would be sufficient to also set it to 0, besides clearing prec_opt.
Although, I'm not sure that touching prec here (with va_arg too) is correct. A negative argument should be ignore, which should result in the default precision (0 for integers, 6 for floats).
See the observations in #295. I think that issue should be solved before reevaluating the current one.

dec and end are only ever non-negative, as far as I can tell.
This too:

nanoprintf/nanoprintf.h

Line 510 in caafd3a

#if (NANOPRINTF_CONVERSION_BUFFER_SIZE <= UINT_FAST8_MAX) && (UINT_FAST8_MAX <= INT_MAX)

only involves non-negatives; it should be fine.

[stefano-zanotti-88]

There's more. Here:

nanoprintf/nanoprintf.h

Line 577 in a4a26d0

npf_ftoa_dec_t end, dec; dec = (npf_ftoa_dec_t)spec->prec;

we have a cast from int prec to npf_ftoa_dec_t dec.
However, npf_ftoa_dec_t can be as small as u8, so this cast can truncate the value. The precision is user-provided, and the standard says it's int (though there are issues with INT_MIN, which should be treated as its absolute value). The [0, 255] minimum range for u8 is enough for most normal cases; however, we do have UB if the user exceeds that. We could use saturation here instead of cast, but then we are no longer standard-conformant on this issue. It might be better to just remove this:

nanoprintf/nanoprintf.h

Lines 520 to 524 in a4a26d0

	#if (NANOPRINTF_CONVERSION_BUFFER_SIZE <= UINT_FAST8_MAX) && (UINT_FAST8_MAX <= INT_MAX)
	typedef uint_fast8_t npf_ftoa_dec_t;
	#else
	typedef int npf_ftoa_dec_t;
	#endif

and always use int for end and dec.
I think this makes a difference only on 8-bit systems, where ints are 16/32-bits, but u8 is faster. On Cortex-M, it seems that fast_u8 is always an alias for u32. Even on 16-bit systems, fast_u8 should be an alias for u16, so we have no problems.
See also #298, where I've added many safety checks. We could leave things as they are now, but add a safety check here (when enabled) to fail in case the cast would truncate the value.
If we do not change anything (except for additional safety checks), I think the effect is this: a large (literal or dynamic) precision can be cast to u8, so it gets "unpredictably" reduced. However, u8 is not negative, so we don't have worse problems down the line: all the code behaves correctly, except that the precision is not what the user wanted.
Maybe this is a good tradeoff for you -- to be "nano, although with corner cases in rarely-hit cases" rather than "pay the price of conformance for cases almost nobody cares about". In this case, I'd still document it as suggested in #299.

Also note the following, though.
I've never needed a large precision (say, > 20) for floats; I've never used it for integers (0-padding is mostly equivalent, except when '#' comes in the mix, or you want mixed space-and-0 padding).
However, a large precision is useful to print non-zero-terminated strings. This is useful when debug-printing buffers (eg UART buffers, which are often not terminated), or when printing a field extracted from some larger string (eg one field of a CSV line, when the extract string is actually just a "view" -- pointer + length -- into the original data).
Still, 255 bytes is often enough.

[charlesnicholson]

Hi @stefano-zanotti-88 - sorry for the delayed response on this and the other issues / PRs; I'll have time this weekend to dive back in :)