ElementsProject
fuzz-tests: add test for amount-{sat, msat} arithmetic
The fuzz-amount test doesn't fuzz the arithmetic operations for struct amount_sat and struct amount_msat. Add a test for the same.
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:
- [x] The changelog has been updated in the relevant commit(s) according to the guidelines.
- [x] Tests have been added or modified to reflect the changes.
- [x] Documentation has been reviewed and updated as needed.
- [x] Related issues have been listed and linked, including any that this PR closes.
Hey @morehouse , running this test throws the following errors:
common/amount.c:359:18: runtime error: -nan is outside the range of representable values of type 'unsigned long'common/amount.c:345:23: runtime error: -inf is outside the range of representable values of type 'unsigned long'common/amount.c:345:23: runtime error: -1.10313e+217 is outside the range of representable values of type 'unsigned long'
among a few others, which are probably due to faulty harness logic. The above failures happen in the following two functions:
WARN_UNUSED_RESULT bool amount_msat_scale(struct amount_msat *val,
struct amount_msat msat,
double scale)
{
double scaled = msat.millisatoshis * scale;
/* If mantissa is < 64 bits, a naive "if (scaled >
* UINT64_MAX)" doesn't work. Stick to powers of 2. */
if (scaled >= (double)((u64)1 << 63) * 2)
return false;
345 val->millisatoshis = scaled;
return true;
}
WARN_UNUSED_RESULT bool amount_sat_scale(struct amount_sat *val,
struct amount_sat sat,
double scale)
{
double scaled = sat.satoshis * scale;
/* If mantissa is < 64 bits, a naive "if (scaled >
* UINT64_MAX)" doesn't work. Stick to powers of 2. */
if (scaled >= (double)((u64)1 << 63) * 2)
return false;
359 val->satoshis = scaled;
return true;
}
Has the fuzzer found an actual bug or is it just another impossible edge case?
Has the fuzzer found an actual bug or is it just another impossible edge case?
I think technically these are bugs -- amount_msat_scale and amount_sat_scale are supposed to return false when scaling fails and already do this for the positive-overflow case. Since there's no usage comment requiring scale to be positive, probably the functions should handle negative overflow as well.
In practice, all current usages of these functions in the codebase have positive values for scale, so the bug can never manifest in the existing codebase.
In practice, all current usages of these functions in the codebase have positive values for
scale, so the bug can never manifest in the existing codebase.
Makes me wonder why scale's type isn't unsigned in the first place...
Makes me wonder why
scale's type isn't unsigned in the first place...
Probably because there's no way to represent an unsigned float.
Hey @morehouse,
As we discussed over email, I spent some time last week investigating the potential bug caused by an assertion failure in amount_msat_sub_fee() to determine whether it could be triggered externally. I grepped the codebase for all callers of amount_msat_sub_fee() and found that the only caller resides in plugins/askrene/refine.c. This, in turn, ultimately traces back to askrene’s getroutes command.
To test this path, I added a user-level Python test in tests/test_askrene.py to try passing the error-causing parameters to amount_msat_sub_fee(). I found that any parameter values that get too large are truncated to a set of ‘safe values’, making it impossible to reproduce the breakage through this command.
In summary, it doesn’t appear that any external message can trigger the failure seen in the fuzz test. I’ve pushed the test in case you’d like to take a look.
The corpus for this test is now generated with the fix in #8306 applied, so that PR needs to be merged before this one can.