Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: write called without restricting size #4

Merged
merged 1 commit into from
Mar 18, 2024
Merged

fix: write called without restricting size #4

merged 1 commit into from
Mar 18, 2024

Conversation

gaukas
Copy link
Contributor

@gaukas gaukas commented Mar 16, 2024

This commit reverts the change by switching back to real-time allocated buffer and fixed a bug caused by writing dupBuf instead of dupBuf[:len(b)].

@gaukas
fix: write called without restricting size
1298a22 
This commit reverts the change by switching back to real-time allocated buffer and fixed a bug caused by writing `dupBuf` instead of `dupBuf[:len(b)]`.

Signed-off-by: Gaukas Wang <[email protected]>
@gaukas gaukas requested a review from erikziyunchi as a code owner March 16, 2024 05:57
@gaukas gaukas self-assigned this Mar 16, 2024
@gaukas
Copy link
Contributor Author

gaukas commented Mar 16, 2024

I'm seriously considering adding a test to check if the built .wasm artifact will actually run on WATER. Apparently this bug should have been detected much earlier.

erikziyunchi
erikziyunchi approved these changes Mar 16, 2024
View reviewed changes
Copy link
Member

@erikziyunchi erikziyunchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for noticing and fixing it! I think we should definitely make a set of tests including edge tests to run on changed .wasm files.

@gaukas
Copy link
Contributor Author

gaukas commented Mar 16, 2024 •
edited
Loading

Also we might want to retract all old versions tagged/released in this repo. I found this bug was actually introduced in gaukas/watm and got carried here, i.e. it exists from the very beginning of refraction-networking/watm. 😞 *sigh*

Actually we can just put up an announcement instead of retraction. I realized the package itself is fine, just the reverse.go.wasm artifact being flawed.

@gaukas gaukas merged commit a4098d9 into master Mar 18, 2024
8 checks passed
@gaukas gaukas deleted the fix-bad-buf branch March 21, 2024 17:59
@gaukas gaukas added the bug Something isn't working label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikziyunchi erikziyunchi erikziyunchi approved these changes

Assignees

@gaukas gaukas

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gaukas @erikziyunchi