common: json_to_s64 accept only valid JSON numbers

[guibsa]

bool str_to_s64 parses a decimal s64 from exactly buflen bytes; false on bad chars, overflow, leading +, hex like '0x...' . Similar behavior of json_to_u64 and str_to_u64. Tests added. Partially fixes #9377

Important

26.09 FREEZE August 5th: Non-bugfix PRs not ready by this date will wait for 26.12.

RC1 is scheduled on August 17th

The final release is scheduled for September 7th.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

• The changelog has been updated in the relevant commit(s) according to the guidelines.
• Tests have been added or modified to reflect the changes.
• Documentation has been reviewed and updated as needed.
• Related issues have been listed and linked, including any that this PR closes.
• Important All PRs must consider how to reverse any persistent changes for tools/lightning-downgrade

[Andezion]

run-utils-str_to_s64.c has no direct test like assert(!str_to_s64("+123", 3, &val)) or assert(!str_to_s64("0x1A", 4, &val)). it only tests --123 and +-123 (mixed/double signs), which is a different case, maybe worth adding something like +123 and a hex string

[Markadrian6399]

If I'm following correctly, the old json_to_s64 copied the token into a temp buffer and handed it to strtoll with base 0, which meant hex, octal, leading whitespace and a leading + all got through. This PR replaces that with a hand written str_to_s64 that walks the buffer digit by digit and rejects anything that isn't a plain decimal, matching how str_to_u64 already behaves. Is that a fair summary of the intent?

A few places I'd appreciate more context on:

1. The negative branch uses val < (INT64_MIN + digit) / 10 and builds the result as val * 10 - digit. My understanding is that accumulating negatively is what lets INT64_MIN be reached, since negating a positive magnitude at the end would overflow on that one value. Is that the reasoning, or is there something else it's protecting against?

2. Leading zeros are accepted ("000123" and the 21 zero case in the tests). JSON itself doesn't allow those, so I assume this is a deliberate choice to stay consistent with str_to_u64 rather than an oversight. Could you confirm?

3. Since the old parser was more permissive, is there a risk of breaking existing callers that were passing values like +5 or 0x10 to an s64 RPC field? I wasn't sure whether those paths exist in tree or whether they'd already have been rejected earlier by jsmn.

4. The issue mentions json_to_double as well. Is that intentionally out of scope here because floats need different handling, or is it queued for a follow up?

5. Minor process question: the checklist marks the changelog as done, but I couldn't find a Changelog- line in the commit message. Is one expected for a change that tightens accepted input, or is that only for user visible behaviour?

[guibsa]

If I'm following correctly, the old json_to_s64 copied the token into a temp buffer and handed it to strtoll with base 0, which meant hex, octal, leading whitespace and a leading + all got through. This PR replaces that with a hand written str_to_s64 that walks the buffer digit by digit and rejects anything that isn't a plain decimal, matching how str_to_u64 already behaves. Is that a fair summary of the intent?

Yes, it's considering acceptable also a minus signal and leading zeros are discarded.

A few places I'd appreciate more context on:

1. The negative branch uses `val < (INT64_MIN + digit) / 10` and builds the result as `val * 10 - digit`. My understanding is that accumulating negatively is what lets `INT64_MIN` be reached, since negating a positive magnitude at the end would overflow on that one value. Is that the reasoning, or is there something else it's protecting against?

yes, just that, INT64_MIN has a larger absolute value than INT64_MAX:
INT64_MAX = 9223372036854775807
INT64_MIN = -9223372036854775808

2. Leading zeros are accepted (`"000123"` and the 21 zero case in the tests). JSON itself doesn't allow those, so I assume this is a deliberate choice to stay consistent with `str_to_u64` rather than an oversight. Could you confirm?

yes, the specification says: "A number is a sequence of decimal digits with no superfluous leading zero.". Looking at str_to_u64 tests, this appears to be intentional. Despite the specification, I tried to maintain compatibility with existing code pattern.

3. Since the old parser was more permissive, is there a risk of breaking existing callers that were passing values like `+5` or `0x10` to an s64 RPC field? I wasn't sure whether those paths exist in tree or whether they'd already have been rejected earlier by jsmn.

Yes, that is possible. The same could happen to str_to_u64.

4. The issue mentions `json_to_double` as well. Is that intentionally out of scope here because floats need different handling, or is it queued for a follow up?

It is out of scope at the moment.

5. Minor process question: the checklist marks the changelog as done, but I couldn't find a `Changelog-` line in the commit message. Is one expected for a change that tightens accepted input, or is that only for user visible behaviour?

Yes, it is missing, i will fix that.