Skip to content

hal: Update matrix keyboard and lcd components to become 64-bit clean - #4744

Open
BsAtHome wants to merge 1 commit into
LinuxCNC:masterfrom
BsAtHome:hal64_matrixkb-lcd
Open

BsAtHome wants to merge 1 commit into
LinuxCNC:masterfrom
BsAtHome:hal64_matrixkb-lcd

Conversation

@BsAtHome

Copy link
Copy Markdown
Contributor

This is a continuation of making components 64-bit clean. The matrix_kb component needed some extra attention because the rowshift based keycode mask calculation was 32-bit. This has been altered to be size-agnostic. Both matrix_kb and lcd now check the return value of hal_malloc().

Comment thread src/hal/components/lcd.c
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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • %f with |value| >= 1e15 (default 4 decimals) gives c > MAX_ENTRY, and the goto overflow path then fills * up to the unclamped c, so it writes past buff. 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: %.25f reads past pow10[], and %5.9f (precision not smaller than width) writes before buff.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

@BsAtHome
BsAtHome force-pushed the hal64_matrixkb-lcd branch 2 times, most recently from 1542a7e to 7ebbb30 Compare October 31, 2026 13:22
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.

2 participants