`moz_task::ThreadPtrHolder` passes an invalid C string to `NS_ProxyReleaseISupports`

RESOLVED FIXED in Firefox 68

Status

()

defect
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: lina, Assigned: lina)

Tracking

(Regression)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

This line casts a static string slice to a *const c_char. But that's wrong—the string isn't NUL-terminated, so ProxyReleaseEvent::GetName will read garbage. I'm surprised ASan didn't catch this, because...

I originally used a CString for ThreadPtrHolder::name, but that was also wrong. ProxyReleaseEvent holds on to the name after the ThreadPtrHolder is dropped, leading to this use after free.

We could:

  1. Add a nul_terminate!("foo") macro that asserts a static string doesn't contain embedded NULs, then adds a trailing one. ThreadPtrHolder::new will likewise want to assert that the string is NUL-terminated.

  2. Change ProxyReleaseEvent to hold an nsCString instead of a const char*. We'd need to change Runnable, too.

  3. Remove ThreadPtrHolder::name entirely, and pass ptr::null(). Proxied releases from Rust won't log runnable names, so this is (probably?) not ideal.

  4. Something else?

(In reply to Lina Cambridge (she/her) [:lina] from comment #0)

We could:

  1. Add a nul_terminate!("foo") macro that asserts a static string doesn't contain embedded NULs, then adds a trailing one. ThreadPtrHolder::new will likewise want to assert that the string is NUL-terminated.

I think this is probably the best option. Surely somebody has already written nul_terminate! or something very like it.

  1. Change ProxyReleaseEvent to hold an nsCString instead of a const char*. We'd need to change Runnable, too.

This change would not be desirable, because then every runnable would need to construct and tear down an nsCString, plus potential additional memory allocation. If the nsCString requirement was contained to ProxyReleaseEvent, that would be more doable.

  1. Remove ThreadPtrHolder::name entirely, and pass ptr::null(). Proxied releases from Rust won't log runnable names, so this is (probably?) not ideal.

I'd prefer to avoid this.

OS: Unspecified → All
Hardware: Unspecified → All
Assignee

Comment 2

2 months ago

This is based on the c_str! macro in js/rust/src/heap.rs, but
returns a byte slice instead of a pointer, so that it can be checked in
Rust without dereferencing.

This commit also changes ThreadPtrHolder::new to assert that name
is a valid C string, and removes an unnecessary RefPtr reference in
is_current_thread.

Comment 3

2 months ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b5b95e9afa0
Add an `xpcom::c_str` macro for creating static C strings. r=froydnj
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Depends on: 1546048
You need to log in before you can comment on or make changes to this bug.