fix: normalize 32-bit slice bounds#1680
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a regression where string slicing with Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a regression on 32-bit targets like esp32c3 where using a 64-bit integer for string slicing could lead to failures. While the fix aims to normalize slice bounds, the current implementation uses unsafe integer truncation which bypasses Go's bounds checking mechanism. This introduces a critical vulnerability, allowing for out-of-bounds memory access when using large int64 indices on 32-bit systems. A more robust approach that performs range checks before truncation is required to maintain memory safety. A temporary regression test has been added, and there is one minor suggestion to improve its readability.
| s := "hello" | ||
| var idx int64 = 1 | ||
| tail := s[idx:] | ||
| if len(tail) == 4 && tail[0] == 'e' && tail[1] == 'l' && tail[2] == 'l' && tail[3] == 'o' { |
There was a problem hiding this comment.
For improved readability and conciseness, you can directly compare the tail slice with the expected string literal. This makes the intent of the check clearer.
| if len(tail) == 4 && tail[0] == 'e' && tail[1] == 'l' && tail[2] == 'l' && tail[3] == 'o' { | |
| if tail == "ello" { |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| } else { | ||
| low = b.fitIntSize(low) | ||
| } | ||
| if !high.IsNil() { | ||
| high = b.fitIntSize(high) | ||
| } | ||
| if !max.IsNil() { | ||
| max = b.fitIntSize(max) | ||
| } |
There was a problem hiding this comment.
A brief comment above this block would help future maintainers understand the motivation, e.g.:
| } else { | |
| low = b.fitIntSize(low) | |
| } | |
| if !high.IsNil() { | |
| high = b.fitIntSize(high) | |
| } | |
| if !max.IsNil() { | |
| max = b.fitIntSize(max) | |
| } | |
| } else { | |
| // Normalize bounds to target int width; on 32-bit targets, | |
| // wider integer indices (e.g. int64) must be cast to int | |
| // to match runtime helper signatures (StringSlice, NewSlice3). | |
| low = b.fitIntSize(low) | |
| } | |
| if !high.IsNil() { | |
| high = b.fitIntSize(high) | |
| } | |
| if !max.IsNil() { | |
| max = b.fitIntSize(max) | |
| } |
| func main() { | ||
| c.Printf(c.Str("Hello World\n")) | ||
| s := "hello" | ||
| var idx int64 = 1 |
There was a problem hiding this comment.
Changing the expected output from "Hello World" to "slice64 ok" means the smoke test no longer independently validates basic c.Printf / c.Str functionality. The program still prints "Hello World" first (line 86), but run_emulator_smoke only checks the last non-empty line, so a silent regression in the basic print path would go undetected.
Consider checking for both strings in the output rather than replacing the original validation.
Review SummaryThe core fix in Key suggestions:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1680 +/- ##
=======================================
Coverage 93.16% 93.16%
=======================================
Files 48 48
Lines 13314 13320 +6
=======================================
+ Hits 12404 12410 +6
Misses 724 724
Partials 186 186 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
luoliwoshang
force-pushed
the
wip/fix-32bit-slice-index-regression
branch
from
8074167 to
bbe037c
Compare
luoliwoshang
changed the title
luoliwoshang
changed the title
| } else { | ||
| low = b.fitIntSize(low) | ||
| } | ||
| if !high.IsNil() { | ||
| high = b.fitIntSize(high) | ||
| } | ||
| if !max.IsNil() { | ||
| max = b.fitIntSize(max) | ||
| } |
There was a problem hiding this comment.
Two observations on this block:
- Missing explanatory comment. The root cause (32-bit targets mishandling wider-than-
intbounds) is well described in the PR body but absent from the source. A brief comment here would prevent future contributors from removing or reordering these calls without understanding why they exist. For example:
// Normalize slice bounds to target int width: on 32-bit targets,
// wider types (e.g. int64) must be truncated before passing to
// runtime helpers that expect int-sized arguments (see #1679).- No repeatable unit test. The fix is only exercised by the ESP emulator smoke test, which requires specific toolchains/emulators. Consider adding a
cl/_testgo/or_cmptest/test case withint64slice bounds so the fix is covered bygo test ./...on any host.
| local actual_tail | ||
| actual_tail=$(printf "%s\n" "$normalized_out" | awk 'NF{print}' | tail -n "$n") |
There was a problem hiding this comment.
Nit: actual_tail is filtered through awk 'NF{print}' (stripping blank lines), but normalized_expected is not. This asymmetry works today because the expected strings have no blank lines, but it will silently break if someone adds an expected string containing an intentional blank line — the actual side would strip it while the expected side preserves it, causing a spurious mismatch.
Consider either applying the same awk 'NF{print}' filter to normalized_expected, or adding a comment documenting this intentional asymmetry.
|
The core fix in Two items worth addressing: (1) a brief code comment explaining why |
| @@ -93,10 +117,10 @@ echo "" | |||
| echo "=== ESP Serial Smoke Tests: Build + Emulator Run ===" | |||
There was a problem hiding this comment.
Split test cases and test runner
luoliwoshang
force-pushed
the
wip/fix-32bit-slice-index-regression
branch
from
4b9ad0c to
bcf60e0
Compare
2 participants
Summary
low/high/maxto targetintwidth in SSASlicebefore calling runtime helpersint64string slicing (slice64 ok)Root Cause
On 32-bit targets, passing wider bounds (for example
int64) directly into runtime slice helpers withintsemantics can cause incorrect behavior (including hangs/no auto-exit).Changes
ssa/datastruct.goBuilder.Slice, applyfitIntSizeto non-nillow/high/maxbefore helper calls_demo/embed/test-esp-serial-startup.shint64slicing regression output in generatedmainVerification
go install ./cmd/llgo_demo/embed/test-esp-serial-startup.shHello World+slice64 ok)Fixes #1679