MDEV-38915: Fix signed/unsigned type mismatch in setval() for GET_ULONG

[luckyxhq]

Bug Description

The GET_ULONG case in setval() (in mysys/my_getopt.c, line 751) incorrectly used signed types (long*) and (long) to cast and store the result of getopt_ull(), which returns an unsigned value (ulonglong).

This creates a signed/unsigned type mismatch that triggers implementation-defined behavior when processing large unsigned long values with the high bit set (i.e., values > LONG_MAX). On platforms where sizeof(long) < sizeof(long long) (32-bit systems, Windows x64), this can silently produce wrong stored values for server configuration variables of type GET_ULONG.

Root Cause

A copy-paste inconsistency: the GET_ULONG case in setval() was written with (long*) / (long) casts (matching the GET_LONG case above it), instead of the correct (ulong*) / (ulong) casts.

Evidence: Two other locations handling GET_ULONG in the same file already use the correct unsigned types:

• init_one_value() at line 1334: *((ulong*) variable)= (ulong) getopt_ull_limit_value(...)
• Print code at line 1721: printf("%lu\n", *((ulong*) value))

Fix

Changed the GET_ULONG case in setval() from:

case GET_ULONG:
    *((long*) value)= (long) getopt_ull(argument, opts, &err);
    break;

to:

case GET_ULONG:
    *((ulong*) value)= (ulong) getopt_ull(argument, opts, &err);
    break;

Changes Made

• mysys/my_getopt.c: Fixed (long*) → (ulong*) and (long) → (ulong) in the GET_ULONG case of setval() (line 751)

Testing

• Verified consistency with all 3 other GET_ULONG handling sites in the same file
• The existing unit test suite (unittest/mysys/my_getopt-t.c) includes GET_ULONG tests that continue to pass
• Minimal one-line change with no risk of regression

Impact

• Fixes potential silent data corruption for GET_ULONG server options on affected platforms
• Zero risk to other code paths — only the GET_ULONG branch of setval() is modified
• Aligns setval() with the already-correct behavior in init_one_value() and the print path

[luckyxhq]

@gkodinov All checks have passed successfully. Could you please review and merge this PR when convenient?

[grooverdan]

Nice find with a simple fix. Btw how did you notice this?

[luckyxhq]

I found it while reviewing type handling in setval(). The GET_ULONG branch looked inconsistent because it was casting to long, even though getopt_ull() returns an unsigned value.
That raised a red flag, so I checked similar code paths and saw they consistently used unsigned types, which made the mismatch clear. so i think i should work on it so that why a raised the pr and want to be merge it.

[gkodinov]

Thank you for your contribution! This is a preliminary review.

Some remarks in no particular order:

• processing on our end would be faster if there was a jira filed and mentioned in the topic. I did that for you now. But it takes some time for me.
• please re-base to 10.11 unless you can provide a scenario that this crashes a binary. See the testing remark.
• please provide tests. I can see there's quite a number of GET_ULONG variables around. If neither of these works, please extend the existing unit test. Apparently it doesn't cover this particular case otherwise it would have failed already.

[luckyxhq]

Thank you for the review and detailed feedback!

I’ll address the points you mentioned:
• I’ll rebase the PR onto 10.11 as suggested.
• I’ll add/extend unit tests to cover this specific GET_ULONG case.
• I’ll also check if I can provide a reproducible scenario demonstrating the issue more clearly.

I push the updates shortly. Please let me know if there’s anything else you’d like me to consider.

[CLAassistant]

All committers have signed the CLA.

[grooverdan]

Thank you for the review and detailed feedback!

I’ll address the points you mentioned:
• I’ll rebase the PR onto 10.11 as suggested.

Done. Thanks. I just edited the target branch in the title.

• I’ll add/extend unit tests to cover this specific GET_ULONG case.
• I’ll also check if I can provide a reproducible scenario demonstrating the issue more clearly.

@luckyxhq, did you find a case? Because I'm not sure there is one. The casting of high values to long fall in the undefined behaviour of the C/C++ standards. Even if you make a test case that fails under the prior to commit implementation, because it's undefined behaviour a different compiler or newer version may implement something different. Whatever test you come up with won't be predictable because a compiler can do what it likes within the undefined behaviour. So what you've done is correct, you've adjusted the implementation to fall within the defined behaviour of the C/C++ standard.

So its my opinion that this can change without a test case. Its passed all the existing tests ok and there will probably be some high values of LONG in those that have been tested to product the same result. Are you happy with this @gkodinov ?

@luckyxhq Can you include the MDEV-38915: as the prefix in the title of the commit message and force push? I'm happy with this to be merged after that happens. So after that, its on @gkodinov.