[tor-bugs] #24031 [Core Tor/Tor]: Protover.rs could use a better algorithm

[Tor Bug Tracker & Wiki]

#24031: Protover.rs could use a better algorithm
-------------------------------------------------+-------------------------
 Reporter:  nickm                                |          Owner:  isis
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  Very High                            |      Milestone:  Tor:
                                                 |  0.3.3.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  0.3.3.1-alpha
 Severity:  Normal                               |     Resolution:
 Keywords:  rust, 033-must, protover, security,  |  Actual Points:  5
  033-triage-20180326, 033-included-20180326     |
Parent ID:                                       |         Points:  1
 Reviewer:  nickm                                |        Sponsor:
                                                 |  SponsorM-can
-------------------------------------------------+-------------------------
Changes (by isis):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:15 chelseakomlo]:
 > Hi Isis,
 >
 > This looks great! Thanks for doing this and making these improvements,
 it is much cleaner!

 Hey! Thanks for the review!

 > There are a few things I noticed when looking at this, they are mostly
 nits. I'll do a deeper look by the end of this week, but this is looking
 significantly better.
 >
 > 1. If possible, it would be ideal to keep all FFI/C related code in
 ffi.rs. This way we can keep Rust/C code cleanly separated and if Rust
 needs to use these functions in the future, we don't need to do any
 refactoring or translation (for example, compute_for_old_tor and
 get_supported_protocols). It looks like in at least once place we get a
 string as a cstring in Rust (in supported for ProtoEntry) and then
 translate into a string- we should just keep this all as strings in Rust
 and then only translate in ffi.rs when we actually want to send something
 to C.

 So the reasoning for this is that static strings should live in the module
 which owns them. (This code is from #25185.) Since we needed a generalised
 way to work with static strings between both C and Rust without
 (re)allocations, in that ticket I created the `cstr!` macro, but a caveat
 is that it ''must'' take a string literal in order to create the `CStr`
 (`&str` is to `String` as `&CStr` is to `CString`, i.e. the static, not-
 heap-allocated equivalent), meaning that we can't assign it to a variable
 first and then put it into the macro elsewhere. E.g. it ''would'' be nicer
 if the `protover::get_supported_protocols_cstr()` function actually lived
 in the `protover::ffi` module, but then we'd need to call something from
 `protover::ffi` in the main `protover.rs` module, and that seemed way
 backwards to me?

 So for the `ProtoEntry::supported()` method, it internally gets the
 `&CStr` from `protover::get_supported_protocols_cstr()` and immediately
 calls `.to_str()` on it so that we're working with the underlying `&str`
 in Rust, then it parses that.

 Basically, the problem is that the macro needs to actually have literally
 the `&static str` ''inside'' it, it cannot work at compile-time if the
 `&static str` and the call to the macro live in different places, because
 at compile-time when the macro is expanded there isn't yet a symbol table
 available for looking up the `&static str` if it were to live somewhere
 else.

 > 2. I really like how you broke out errors into their own file. Is this
 something we should add to our guide to writing Rust in Tor?

 Probably! I was going to draft it into a guideline, but this will also
 become much better when we can use [https://crates.io/crates/failure
 failure] which is supposed to be 1.0.0 any day now. Adding it for
 `ProtoverError` is a one line addition:

 {{{#!rust
 impl ::failure::Fail for ProtoverError {};
 }}}

 I've made #25628 for tracking this.

 > 3. Is generating ToString implementations at runtime a common Rust
 practice? If not, document impl_to_string_for_proto_entry

 It's compile-time! :) To see what it's generating, add this to the top of
 `lib.rs` (and use a nightly compiler):

 {{{#!rust
 #![feature(trace_macros)]

 trace_macros!(true);
 }}}

 I don't know if it's standard to do it for `ToString`, but people
 generally use macros to generate trait implementations for types that
 should have the same implementation to avoid copy-pasting code that could
 get out of sync. (I see people do it a lot for `Display`/`Debug`/`Clone`,
 etc.)

 > 4. `supported` for ProtoEntry maybe should return an actual error
 instead of an empty string

 It is! `ProtoEntry::supported()` returns `ProtoverError` because it sets
 the `supported` variable to an empty string if there was an error, and
 then returns `supported.parse()`.

 {{{#!rust
 impl ProtoEntry {
     // [...]
     pub fn supported() -> Result<Self, ProtoverError> {
         let supported_cstr: &'static CStr =
 get_supported_protocols_cstr();
         let supported: &str = match supported_cstr.to_str() {
             Ok(x)  => x,
             Err(_) => "",
         };
         supported.parse()
     }
 }
 }}}

 (`supported.parse()` is calling `impl FromStr for ProtoEntry`, and a blank
 string is not a valid `ProtoEntry` as the
 `test_protoentry_from_str_empty()` shows.)

 Do you think it might read better if I refactored setting `supported` to
 be like this?

 {{{#!rust
         let supported: &str = supported_cstr.to_str().unwrap_or("");
 }}}

 -----

 Setting as needs_review again? Not sure what the correct ticket state is?
 Feel free to change it back if you still think revision is necessary
 somewhere!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/24031#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online