[Ramji-Purwar]

Fixes #27117

strtol(), strtoll(), strtoul() and strtoull() returned wrong results for some valid inputs.

Problems before this PR

Call	Before	After
strtol("0x1A", NULL, 16)	0	26
strtol("-0x10", NULL, 16)	0	-16
strtoul("+42", NULL, 10)	0	42
strtoul("-1", NULL, 10)	0	ULONG_MAX
strtoull("99999999999999999999", NULL, 10)	LLONG_MAX	ULLONG_MAX
strtoll("-9223372036854775807", NULL, 10)	LLONG_MIN, errno = ERANGE	-9223372036854775807

This also affected programs that ship with Serenity:

• printf '%u\n' +42 printed 0. It now prints 42.
• HexEditor's "Fill Selection" with 0xff filled the selection with 00. It now fills it with FF.

What caused the problems

• strtoll() and strtoull() each had their own copy of the code that parses the base and the 0x prefix, and the two copies were different. strtoull() skipped 0x when the base was 16, but strtoll() only did that when the base was 0. With base 16, strtoll() parsed the 0, stopped at the x and returned 0. strtol() calls strtoll(), so it had the same problem.
• strtoull() never parsed the optional sign, unlike strtoll(). A leading + or - was treated as an invalid character, so no digits were read and it returned 0.
• When strtoull() overflowed, it returned LONG_LONG_MAX, which iype, instead of ULONG_LONG_MAX.
• For negative numbers, the digit parser used LLONG_MIN % base as the limit for the last digit. In C++ that remainder is negative (-8 for base 10), so every last digit failed the digit <= limit check and the number was reported

Changes

• strtoll() and strtoull() now share one helper for the base and strtoll() also accepts the prefix when the base is 16. The prefixis only skipped if a hex digit follows it, so "0x" on its own parses as 0, as POSIX requires.
• strtoull() now accepts a leading + or - (a - negates the r returns ULLONG_MAX instead of LLONG_MAX on overflow.
• Fixed the limit for the last digit of negative numbers, which made strtoll() treat values from -9223372036854775800 to -9223372036854775808 as an overflow.
• Added Tests/LibC/TestStrtol.cpp.

[Hendiadyoin1]

I think I understand what the the change wants to achieve,
but I am not sure what this has to do with LLONG_MIN specifically, as this seemed to have been an issue with all negative numbers?

TBF I have no clue what this is trying to achieve with the cutoff anyway

[Hendiadyoin1]

Ah read the PR description a bit more, now I understand a bit more,
IMO the class likely needs a rewrite....

[Ramji-Purwar]

Only numbers whose first digits are exactly the cutoff (LLONG_MIN / base, i.e. -922337203685477580 for base 10) reach the comparison against m_max_digit_after_cutoff. All other negative numbers are either clearly below the cutoff or already past it, so they never hit the broken check. That's why only -9223372036854775800 to -9223372036854775808 were affected.

I agree that NumParser could be simplified. I'd prefer to keep this PR a minimal fix, but I'm happy to do a cleanup in a follow-up PR, or here if you'd rather have it in this one.

[Hendiadyoin1]

It would have been nice to have the tests in the feature commits, but this way is also fine I guess

[Ramji-Purwar]

Thanks! I can move the tests into the commits that fix each case if you'd like, just let me know.