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 weak-ref by-value function parameter UAF #2677

Closed
wants to merge 1 commit into from
Closed

Fix weak-ref by-value function parameter UAF #2677

wants to merge 1 commit into from

Conversation

mikialex
Copy link

The #2447 not considered the by-value function parameter case. This MR try fix that.

All xxx.ptr = 0; will now called by __destroy_into_raw and unregister the finalizer.

@mikialex
Copy link
Author

Could someone help checking why ci failed. Maybe the error not related to this change

danielhenrymantilla
danielhenrymantilla approved these changes Sep 10, 2021
View reviewed changes
Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

Indeed, the CI failure seems to stem from a change in the compiler diagnostics; UI tests are generally brittle because of this. Someone ought to take the top of master, run the UI tests with the override setting enabled, so as to update the error message expectations, and submit a PR with those changes, so that you can afterwards rebase and fix that.

Regarding this PR, as the author of the one that fixed the self case (sorry I did not notice it occurred for non self values as well), the changes LGTM ✅

@alexcrichton
Copy link
Contributor

Thanks for this! Can you add some tests specifically for this bug as well?

@mikialex
Copy link
Author

According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry, the time when FinalizationRegistry cleanup logic triggered is unpredictable. So I think it's hard to add test logic into CI, because your can't await GC happens.

For reference I create a repo simply for reproducing this bug and test the above fix: https://github.com/mikialex/wasm-bindgen-reproduce-2677

@alexcrichton
Copy link
Contributor

Could the test you mentioned get added to this test suite? Even if it's not deterministically exercising the bug it's better than nothing I think.

@mikialex
Copy link
Author

I'm trying to add some test but have some issues when figuring out how to run test suite on my machine.

Accroding to this https://rustwasm.github.io/docs/wasm-bindgen/contributing/testing.html#wasm-tests-on-node-and-headless-browsers and this https://rustwasm.github.io/wasm-bindgen/wasm-bindgen-test/browsers.html#appendix-testing-in-headless-browsers-without-wasm-pack, I tried SAFARIDRIVER=/usr/bin/safaridriver cargo test --target wasm32-unknown-unknown and cargo test --target wasm32-unknown-unknown they produce this error:

Running `target/debug/wasm-bindgen-test-runner /Users/mikialex/dev/wasm-bindgen/target/wasm32-unknown-unknown/debug/deps/headless-4f54a9baec924af2.wasm`
Set timeout to 20 seconds...
Running headless tests in Safari on `http://127.0.0.1:51646/`
Try find `webdriver.json` for configure browser's capabilities:
Ok
driver status: signal: 9                          
Error: non-200 response code: 500                 
<html>
<head>
 <title>500 Internal Privoxy Error</title>
 <link rel="shortcut icon" href="http://config.privoxy.org/error-favicon.ico" type="image/x-icon"></head>
<body>
<h1>500 Internal Privoxy Error</h1>
<p>Privoxy encountered an error while processing your request:</p>
<p><b>Could not load template file <code>no-server-data</code> or one of its included components.</b></p>
<p>Please contact your proxy administrator.</p>
<p>If you are the proxy administrator, please put the required file(s)in the <code><i>(confdir)</i>/templates</code> directory.  The location of the <code><i>(confdir)</i></code> directory is specified in the main Privoxy <code>config</code> file.  (It's typically the Privoxy install directory, or <code>/etc/privoxy/</code>).</p>
</body>
</html>

error: test failed, to rerun pass '--test headless'

so, did I miss something important? I assum this will execute the tests in tests folder.

@alexcrichton
Copy link
Contributor

I'd recommend running the --test wasm tests which should only require node installed with a somewhat recent version

@cycraig cycraig mentioned this pull request Mar 7, 2022
Merged
10 tasks
@ept ept mentioned this pull request May 25, 2022
Closed
@Alfred-Mountfield Alfred-Mountfield mentioned this pull request Aug 29, 2022
Open
robertknight added a commit to robertknight/rten that referenced this pull request Dec 9, 2022
@robertknight
Pass wasm_api::Tensor into TensorList::push by-reference
f992cca 
Work around a bug [1] in wasm-bindgen when passing structs by-value into
methods, by passing `wasm_api::Tensor`s by-reference. In order to do this
without copying the underlying tensor, make `wasm_api::Tensor` use an Rc to
manage its output reference.

Alternative solutions would be to fix the issue in wasm-bindgen upstream or
to patch generated JS code snippets like this:

```
var ptr0 = someArg.ptr;
somePtr.ptr = 0;
```

To be like this instead:

```
var ptr0 = someArg.__destroy_into_raw();
```

[1] rustwasm/wasm-bindgen#2677
@alexjg
Copy link

alexjg commented Feb 3, 2023

Am I correct in thinking that this issue is fixed by 4458587 ?

@Liamolucko
Copy link
Collaborator

Am I correct in thinking that this issue is fixed by 4458587 ?

Yes, I think so.

@Liamolucko Liamolucko closed this Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielhenrymantilla danielhenrymantilla danielhenrymantilla approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mikialex @alexcrichton @alexjg @Liamolucko @danielhenrymantilla