Opened 2 weeks ago

Closed 8 days ago

#25127 closed defect (fixed)

Rust implementation of protover_get_supported_protocols() leaks memory

Reported by: nickm Owned by: isis
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.2.1-alpha
Severity: Normal Keywords: rust, protover, leak
Cc: chelseakomlo, catalyst, ffmancera@…, Samdney Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorM

Description

The src/rust/protover/ffi.rs version of protover_get_supported_protocols returns a newly allocated char*. But the C version of it returns a const char *, and doesn't allocate. This difference causes a memory leak when the C code uses the rust.

Child Tickets

Change History (24)

comment:1 Changed 2 weeks ago by nickm

Cc: chelseakomlo added

I think we could use some Rust guidance about what's the right way to do this. We could maybe make a singleton copy of the string? And then free it on exit somehow?

comment:2 Changed 2 weeks ago by Hello71

Owner: set to Hello71
Status: newassigned

I will do this.

comment:3 Changed 2 weeks ago by Hello71

Status: assignednew

I will not do this. :)

It would be best to join with " " at compile time, but I don't know if that's even possible with Rust macros. Alternatively, import lazy_static, but I got yelled at last time I was asking about TOR_RUST_DEPENDENCIES.

comment:4 Changed 2 weeks ago by catalyst

Cc: catalyst added

comment:5 Changed 2 weeks ago by Hello71

Owner: Hello71 deleted
Status: newassigned

comment:6 Changed 2 weeks ago by Hello71

Status: assignednew

because trac.

comment:7 Changed 2 weeks ago by ffmancera

Cc: ffmancera@… added

comment:8 Changed 2 weeks ago by catalyst

I'm still quite new to Rust so I'm not sure of the best way to handle this. I do observe that protover.rs makes a list of strings as primary, while protover.c makes the concatenated (space-separated) single string primary. I'm not sure of the history or rationale behind this discrepancy.

There's also protover_free_all() that we could use to handle freeing of any long-lived or singleton objects created at run time.

comment:9 Changed 2 weeks ago by Samdney

Cc: Samdney added

comment:10 Changed 2 weeks ago by isis

Owner: set to isis
Status: newassigned

I believe the fix is changing to doing

use std::ffi::CStr;

const C_COMPATIBLE_AND_LIVES_FOREVER: &'static CStr = CStr::from_bytes_with_nul_unchecked(b"STUFF GOES HERE\0");

and then passing to C with tor_c_function(C_COMPATIBLE_AND_LIVES_FOREVER.as_ptr()).

I'll go patch it and valgrind it for leaks.

Last edited 2 weeks ago by isis (previous) (diff)

comment:11 Changed 2 weeks ago by teor

This issue also affects protover_compute_for_old_tor().
All the other strings passed from Rust to C are freed correctly.

I think we need a new rule to catch bugs like this one:
"The C unit tests must call every Rust ffi function."

comment:12 Changed 2 weeks ago by isis

Hi! I have a patch to fix that leak, but then noticed something was still leaking, and then noticed teor's comment above. The TravisCI is passing again at least, so that's nice.

I'll fix the rest of the leak tomorrow and add it to the bug25127 branch and try to make some utilities/abstractions for working with static string-like things better between Rust and C.

comment:13 Changed 13 days ago by catalyst

I found c_str_macro which looks like it might help eliminate the explicit NUL in the string literal. I'm not sure if we want to depend on that crate though.

comment:14 Changed 12 days ago by isis

Sponsor: SponsorM
Status: assignedneeds_review
Version: Tor: 0.3.2.1-alpha

There's patches in my bug25127 branch. I'm not sure if I like what I came up with though. It might be better to make a macro to add the nul byte, similar to the one catalyst linked to, but probably without the as *const ::std::os::raw::c_char part. Opinions?

Version 1, edited 12 days ago by isis (previous) (next) (diff)

comment:15 Changed 12 days ago by teor

A standard macro that we can use in all our crates would be really helpful.

comment:16 Changed 11 days ago by catalyst

Status: needs_reviewmerge_ready

I'm ok with merging these patches as is if we open a new ticket to abstract the creation of string or byte literals that are more C-friendly. Getting Travis builds working again is important.

comment:17 Changed 11 days ago by nickm

I've merged isis's branch above, with a couple of small tweaks (see d8307cb0e99d28daa4011e4e9d94e3f8c56cba23 and af049657eb4426f4c1e7c8aa603c6303c9a884cf). Now I'll try to merge #25067. Let's iterate from there.

comment:18 Changed 11 days ago by isis

Thanks! I'll rebase and make a new branch for the utilities to avoid this type of issue again.

comment:19 in reply to:  18 Changed 11 days ago by isis

Replying to isis:

Thanks! I'll rebase and make a new branch for the utilities to avoid this type of issue again.


The ticket for this is #25185.

comment:20 Changed 11 days ago by isis

Resolution: fixed
Status: merge_readyclosed

tiny nitpick/note: The code from d8307cb0e99d28daa4011e4e9d94e3f8c56cba23 and d8307cb0e99d28daa4011e4e9d94e3f8c56cba23 has an unwrap() in an FFI function, which if it were to panic!() would be UB. However, the unwrap() and potential panic!() is, I think, the same level of "unsafety" as writing unsafe{} and making a bug, given that the same checks are still in place and we're operating with the same set of assumptions.

comment:21 Changed 10 days ago by nickm

oh! I hadn't known that a ffi-panic would be UB. We should fix that too, if there's an easy way around it.

comment:22 in reply to:  21 Changed 10 days ago by isis

Resolution: fixed
Status: closedreopened

Replying to nickm:

oh! I hadn't known that a ffi-panic would be UB. We should fix that too, if there's an easy way around it.


Okay, it's pretty easy since—wherever else in the protover code that stuff can go wrong—we just return/use empty strings, so we could just change the unwrap()s to unwrap_or(""). There is a patch in my bug25127_redux branch.

comment:23 Changed 10 days ago by isis

Status: reopenedneeds_review

comment:24 Changed 8 days ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged!

Note: See TracTickets for help on using tickets.