Fix sigreturn for x86

[CvvT]

According to Linux, esp - 8 should point to SignalFrame instead of LegacyContext. We could also remove the - 8 and following wrapping_add directly but keep it just to be consistent with Linux.

There are some potential overflow issues where we perform some arithmetic operations on user provided input. We should always use checked_* or wrapping_*. There are likely more similar issues in the codebase, and this PR only fixes the ones related to sigreturn.

[github-actions]

🤖 SemverChecks 🤖 No breaking API changes detected

Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered.

[CvvT]

There are some potential overflow issues where we perform some arithmetic operations on user provided input. We should always use checked_* or wrapping_*. There are likely more similar issues in the codebase, and this PR only fixes the ones related to sigreturn.

Just learnt that Rust detects integer overflows in debug mode but does wrapping in release mode. Technically, it should be fine without the fix as we don't expect malicious input in debug mode.

[jaybosamiya-ms]

Thanks for mentioning the arithmetic thing! Indeed we have some number of arith issues that have slipped in over time and we should do a cleanup pass at some point. For now, the semantics seem reasonable enough (i.e., we can just hard set them to checked/wrapping via a compile time flag depending on what we need) but a cleanup would be good. The harder part is longer-term enforcement which I don't have a good solution for (there are some very good reasons to use normal arith, and we can't just ban +/-/...). Also, briefly: when looking for places to cleanup, we should also look for places where saturating_* is the right move (not just checked_* or wrapping_*). For example, one of the changes I'm working on rn has such a natural use case (let available = data.len().saturating_sub(offset);).

[wdcui]

I asked Claude to find similar bugs in other places and got the following:

Priority | File | Line(s) | Issue
HIGH | signal/x86.rs | 145-146, 175-176 | + should be wrapping_add (inconsistent with x86_64)
MEDIUM | signal/mod.rs | 266, 353 | + on user-provided alt-stack values
MEDIUM | process.rs | 471, 478, 494 | + with user-controlled futex_offset
LOW | signal/x86_64.rs | 23-25 | Implicit offset assumption (fragile, not broken today)

[CvvT]

I asked Claude to find similar bugs in other places and got the following:

Priority | File | Line(s) | Issue HIGH | signal/x86.rs | 145-146, 175-176 | + should be wrapping_add (inconsistent with x86_64) MEDIUM | signal/mod.rs | 266, 353 | + on user-provided alt-stack values MEDIUM | process.rs | 471, 478, 494 | + with user-controlled futex_offset LOW | signal/x86_64.rs | 23-25 | Implicit offset assumption (fragile, not broken today)

I also asked agent to do the same thing, and it gave some reasonable results. The bigger question is how to find all similar issue and how to prevent it from happening in the future. It would be better if we have a systematic approach. Meanwhile, I can submit a separate PR to fix all issues that AI found.

[CvvT]

Thanks for mentioning the arithmetic thing! Indeed we have some number of arith issues that have slipped in over time and we should do a cleanup pass at some point. For now, the semantics seem reasonable enough (i.e., we can just hard set them to checked/wrapping via a compile time flag depending on what we need) but a cleanup would be good. The harder part is longer-term enforcement which I don't have a good solution for (there are some very good reasons to use normal arith, and we can't just ban +/-/...). Also, briefly: when looking for places to cleanup, we should also look for places where saturating_* is the right move (not just checked_* or wrapping_*). For example, one of the changes I'm working on rn has such a natural use case (let available = data.len().saturating_sub(offset);).

I agree long-term enforcement would be difficult. It would be nice if we can differentiate user input from validated data, then we will be cautious about any operations on the non-validated data.

[wdcui]

Thanks for mentioning the arithmetic thing! Indeed we have some number of arith issues that have slipped in over time and we should do a cleanup pass at some point. For now, the semantics seem reasonable enough (i.e., we can just hard set them to checked/wrapping via a compile time flag depending on what we need) but a cleanup would be good. The harder part is longer-term enforcement which I don't have a good solution for (there are some very good reasons to use normal arith, and we can't just ban +/-/...). Also, briefly: when looking for places to cleanup, we should also look for places where saturating_* is the right move (not just checked_* or wrapping_*). For example, one of the changes I'm working on rn has such a natural use case (let available = data.len().saturating_sub(offset);).

I agree long-term enforcement would be difficult. It would be nice if we can differentiate user input from validated data, then we will be cautious about any operations on the non-validated data.

I have been thinking that we could leverate GH agentic workflows to run checks of various patterns periodically to look for bugs introduced in new code. This could be one of them. And we can add other checks as well.

[CvvT]

Thanks for mentioning the arithmetic thing! Indeed we have some number of arith issues that have slipped in over time and we should do a cleanup pass at some point. For now, the semantics seem reasonable enough (i.e., we can just hard set them to checked/wrapping via a compile time flag depending on what we need) but a cleanup would be good. The harder part is longer-term enforcement which I don't have a good solution for (there are some very good reasons to use normal arith, and we can't just ban +/-/...). Also, briefly: when looking for places to cleanup, we should also look for places where saturating_* is the right move (not just checked_* or wrapping_*). For example, one of the changes I'm working on rn has such a natural use case (let available = data.len().saturating_sub(offset);).

I agree long-term enforcement would be difficult. It would be nice if we can differentiate user input from validated data, then we will be cautious about any operations on the non-validated data.

I have been thinking that we could leverate GH agentic workflows to run checks of various patterns periodically to look for bugs introduced in new code. This could be one of them. And we can add other checks as well.

I just created one on a forked repo: https://github.com/CvvT/litebox/tasks/211f64ae-2e6f-4d63-9f09-7ef03a8104e8?author=CvvT
Let's see how it works out.