Repository navigation
Conversation
| unsigned int v = hal_get_ui32(val->u); | ||
| rtapi_uint v = hal_get_uint(val->u); | ||
|
|
||
| if (c < 1) c = num_digits_baseN(v, base); |
There was a problem hiding this comment.
Now that the full 64-bit value reaches num_digits_baseN(), can m *= base wrap? With %x and a value >= 16^15, I think m becomes 0 and the loop never ends in the RT thread (%o likewise for >= 2^63).
And with %o, a 64-bit value can need 22 digits; is anything clamping c to MAX_ENTRY here, or does the digit loop run past buff?
Counting by division would avoid the wrap:
while (val >= (rtapi_u64)base) {
val /= base;
n++;
}
There was a problem hiding this comment.
Good one. Changed.
There are still some "gaps" in the conversion. The floating point conversion uses a "blind" conversion to integer and it hopes to fit into a 64-bit value when it counts the digits. Don't know if it is worth the hassle to fix all the problems in lcd.
There was a problem hiding this comment.
Thanks, the hang is gone. The second half of my question still stands, I think: with %o and a value >= 2^60, num_digits_baseN() returns 21 or 22, and nothing clamps c before out[c] = 0, so it writes past buff[MAX_ENTRY + 1]. Would one line after the width calculation be enough? The digit loop already prints * when the value does not fit:
if (c < 1) c = num_digits_baseN(v, base);
if (c > MAX_ENTRY) c = MAX_ENTRY;On the float conversion, I wondered the same, so I pulled parse_fmt() into a small userspace harness with ASan and UBSan. The float path is unchanged from master, so none of this comes from your PR, but I think one part is worth fixing anyway, because a plain pin value reaches it:
%fwith |value| >= 1e15 (default 4 decimals) givesc> MAX_ENTRY, and thegoto overflowpath then fills*up to the unclampedc, so it writes pastbuff. Same for NaN, where the conversion yields 2^63 on x86.- 1e300 and inf print
0.0000: the "blind" conversion you mention is UB and comes back 0 here. - From the format string only:
%.25freads pastpow10[], and%5.9f(precision not smaller than width) writes beforebuff.
Would something like this be enough? It keeps the existing overflow path and only feeds it a clamped width:
if (m < 0) m = 4;
if (m > MAX_ENTRY - 2) m = MAX_ENTRY - 2;
if (c < 1) c = v < 1e19 ? num_digits_baseN(v, base) + m + 1 + s : MAX_ENTRY;
if (c > MAX_ENTRY || c < m + 2 || !(v * pow10[m] < 1e19)){ // then it won't fit, also inf and NaN
if (c > MAX_ENTRY) c = MAX_ENTRY;
tmp = 2;
goto overflow;
}With both changes the harness is clean on 25 cases (64-bit %u %x %o %d, %f from 3.14 to 1e300, inf, NaN, %.25f, %5.9f, %06.2f), and the normal outputs are unchanged. Values that do not fit now show **** instead of corrupting memory or showing 0.0000. I am fine with this here or as a separate PR, whichever you prefer.
There was a problem hiding this comment.
The MAX_ENTRY needs to encompass the %o format and thus needs to be ceil(64/3) = 22 or more.
The FP question,... Well, the thing is that the calculation will overflow badly when the value is >= 2^64 because the digit calculation truncates the actual value in a cast from double to rtapi_u64. The most obvious overflow is from %.21f and beyond because it indexes the pow10[] array out of bounds. So a check on m >= NELEM(pow10)` and go to overflow can trivially prevent that.
You are right that there are more things that need to be done. I'm not sure it is worth the effort to do much more in this PR as the real problem has been present for a long time. FWIW, the code should probably use rtapi_vsnprintf derived code instead.
There was a problem hiding this comment.
Thanks, %o is fine now with MAX_ENTRY 22, the harness agrees.
On the new m check: I think the goto overflow comes before c gets a value, or am I missing something? With %.25f and no width, c is still -1, so out[c] = 0 writes one byte before buff; ASan flags it. Giving it the full width first fixes it in the harness:
if (m >= (int)(sizeof(pow10) / sizeof(*pow10))) {
if (c < 1) c = MAX_ENTRY;
tmp = 2;
goto overflow;
}Agreed on leaving the rest out of this PR, since it is old. For the record, what still shows up in the harness: %f with values >= 1e18 and NaN write past buff through the unclamped c in the overflow path, %5.9f writes before it, and 1e300, inf and %.20f of 1 print zeros.
There was a problem hiding this comment.
Good you are looking. Missed that one.
But is is also an argument to spit the printable string format into fractions at load time that are passed to rtapi_snprintf() part-by-part with the correct argument and the full result constructed that way. It would prevent using the reinvention of the wheel as a square with two sides missing ;-)
1542a7e to
7ebbb30
Compare
7ebbb30 to
ef3492a
Compare
This is a continuation of making components 64-bit clean. The
matrix_kbcomponent needed some extra attention because therowshiftbasedkeycodemask calculation was 32-bit. This has been altered to be size-agnostic. Bothmatrix_kbandlcdnow check the return value ofhal_malloc().