rust protover_all_supported() returns rust-allocated string in *missing_out

Trac:
Username: cyberpunks

changed milestone to %Tor: 0.3.5.x-final

added 033-backport 034-backport 035-must component::core tor/tor fast-fix memory-safety milestone::Tor: 0.3.5.x-final owner::ahf priority::very high protover reporter::cyberpunks resolution::fixed reviewer::catalyst rust severity::normal status::closed type::defect version::tor 0.3.3.1-alpha labels

Trac:
Milestone: N/A to Tor: unspecified

This needs the keywords rust, protover, memory-safety. You can't tor_free() a string allocated by rust code, which is what the C code will immediately try to do.

Trac:
Username: cyberpunks

Thanks for this bug report.

As far as I understand it, it may be ok to allocate in Rust and deallocate in C, as long as they use the same allocator. But, this behaviour is not guaranteed to be safe in future Rust releases: https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandardsRust.md#n365

But even if allocating in Rust and freeing in C was safe, this function is also memory unsafe because:

• *missing_out is allocated in Rust, deallocated in Rust (when the function returns), used in C, and then freed in C
• when missing_out is NULL, Rust still assigns to it

I'll open child tickets for these issues.

Trac:
Milestone: Tor: unspecified to Tor: 0.3.5.x-final
Keywords: N/A deleted, rust, memory-safety, 035-must, protover, fast-fix added

Trac:
Keywords: N/A deleted, 034-backport, 033-backport added

Replying to teor:

As far as I understand it, it may be ok to allocate in Rust and deallocate in C, as long as they use the same allocator. Oh right. Of course currently, they don't.

deallocated in Rust (when the function returns), It actually isn't.

• when missing_out is NULL, Rust still assigns to it You're right. Nice catch!

Trac:
Username: cyberpunks

Rust uses jemalloc to allocate memory by default. No CI has been failing even though test_protover_all_supported calls tor_free() on this jemalloc-allocated pointer though. Is it missing hardening that could catch similar issues in the future?

Trac:
Username: cyberpunks

Replying to cyberpunks:

Replying to teor:

As far as I understand it, it may be ok to allocate in Rust and deallocate in C, as long as they use the same allocator. Oh right. Of course currently, they don't.

We have allocate_and_copy_string() in Rust, which uses tor_malloc(): https://github.com/torproject/tor/blob/master/src/rust/tor_allocate/tor_allocate.rs#L30

We need to update our RustCodingStandards.md to mention allocate_and_copy_string(): https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandardsRust.md#n374

I'll open another ticket.

Replying to cyberpunks:

Rust uses jemalloc to allocate memory by default. No CI has been failing even though test_protover_all_supported calls tor_free() on this jemalloc-allocated pointer though. Is it missing hardening that could catch similar issues in the future?

ASAN would probably help, but it doesn't work with jemalloc, see #27272 (moved).

https://gitgud.io/onionk/tor/compare/master...rust-allsupportednull1

Trac:
Username: cyberpunks

Mark all 035-must tickets as "very high"

Trac:
Priority: Medium to Very High

Assign a couple of quick and easy rust tickets

Trac:
Status: new to assigned
Owner: N/A to ahf

I think this has a patch, so it needs review?

Trac:
Status: assigned to needs_review

Trac:
Reviewer: N/A to nickm

Merged, with a conflict resolved in 289a7dbac32a981897e12a3c250f0b6c67eec809. (Does that look okay?)

Replying to nickm:

(Does that look okay?)

No, that merge completely undid the whole diff in 42558df7c8affeec33e66d987ccf4d632a9d5466 that fixed this bug. It's right back to creating a Rust-allocated CString and then calling CString::into_raw() which as the commit summary notes is not safe.

Trac:
Username: cyberpunks

I've tried to restore the fix in bug27740_035_fix; PR at https://github.com/torproject/tor/pull/458 . How does that look?

Trac:
Reviewer: nickm to N/A

Trac:
Reviewer: N/A to catalyst

Looks good to me!

Trac:
Status: needs_review to merge_ready

Thanks; merged it!

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

mentioned in issue #27805 (moved)

moved to tpo/core/tor#27740 (closed)