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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Lina Butler [:lina]]

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?

Comment 1

[Nathan Froyd [:froydnj]]

(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.

2. 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.

3. 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.

Comment 2

[Lina Butler [:lina]]

Attachment: Bug 1542528 - Add an `xpcom::c_str` macro for creating static C strings. r?froydnj!

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b5b95e9afa0
Add an `xpcom::c_str` macro for creating static C strings. r=froydnj

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4b5b95e9afa0