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.

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

Enable windows-debug github runner #1347

Merged
merged 1 commit into from
Oct 25, 2023
Merged

Conversation

ohodson
Copy link
Contributor

@ohodson ohodson commented Oct 25, 2023

The windows-debug config exposed a UAF for the testUsvPtr() test in
the KJ_TEST("JavaScript USVStrings") since the debug build on Windows
overwrites freed memory which shows up as corruption if used after
free. The issue is described in #1334.

Fix: #1334
Test: bazel test -c dbg --cache_test_results=no //...

The windows-debug config exposed a UAF for the testUsvPtr() test in
the KJ_TEST("JavaScript USVStrings") since the debug build on Windows
overwrites freed memory which shows up as corruption if used after
free. The issue is described in #1334.

Fix: #1334
Test: bazel test -c dbg --cache_test_results=no //...
@ohodson ohodson requested review from a team as code owners October 25, 2023 12:23
Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

I wonder if we should also kill UsvStringPtr from our codebase to avoid falling into this trap again? or is it something that we cannot get rid of?

@ohodson
Copy link
Contributor Author

ohodson commented Oct 25, 2023

I wonder if we should also kill UsvStringPtr from our codebase to avoid falling into this trap again? or is it something that we cannot get rid of?

@jasnell commented in #1334 (comment) that UsvString, and by extension UsvStringPtr are up for retirement.

I think the positive here is that the extra build config found an issue and will likely more of the same if they occur in future.

BTW, 100% open to a better fix if the folks more familiar with these bits see one.

@ohodson ohodson merged commit 95493b4 into main Oct 25, 2023
11 checks passed
@ohodson
Copy link
Contributor Author

ohodson commented Oct 25, 2023

@dom96 , thanks for the review. 👍

@kentonv kentonv deleted the orion/enable-windows-debug branch October 30, 2023 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

workerd: windows-debug build fails string-test
2 participants