1. 06 Apr, 2018 1 commit
  2. 03 Apr, 2018 7 commits
  3. 02 Apr, 2018 32 commits
    • Isis Lovecruft's avatar
      rust: Fix ProtoSet and ProtoEntry to use the same DoS limits as C. · c65088cb
      Isis Lovecruft authored
      Previously, the limit for MAX_PROTOCOLS_TO_EXPAND was actually being applied
      in Rust to the maximum number of version (total, for all subprotocols).
      Whereas in C, it was being applied to the number of subprotocols that were
      allowed.  This changes the Rust to match C's behaviour.
      c65088cb
    • Isis Lovecruft's avatar
      rust: Port all C protover_all_supported tests to Rust. · 4b4e36a4
      Isis Lovecruft authored
      The behaviours still do not match, unsurprisingly, but now we know where a
      primary difference is: the Rust is validating version ranges more than the C,
      so in the C it's possible to call protover_all_supported on a ridiculous
      version range like "Sleen=0-4294967294" because the C uses
      MAX_PROTOCOLS_TO_EXPAND to count the number of *subprotocols* whereas the Rust
      uses it to count the total number of *versions* of all subprotocols.
      4b4e36a4
    • Isis Lovecruft's avatar
      tests: Run all existing protover tests in both languages. · 6739a69c
      Isis Lovecruft authored
      There's now no difference in these tests w.r.t. the C or Rust: both
      fail miserably (well, Rust fails with nice descriptive errors, and C
      gives you a traceback, because, well, C).
      6739a69c
    • Isis Lovecruft's avatar
      tests: Make inline comments in test_protover.c more accurate. · f769edd1
      Isis Lovecruft authored
      The DoS potential is slightly higher in C now due to some differences to the
      Rust code, see the C_RUST_DIFFERS tags in src/rust/protover/tests/protover.rs.
      
      Also, the comment about "failing at the splitting stage" in Rust wasn't true,
      since when we split, we ignore empty chunks (e.g. "1--1" parses into
      "(1,None),(None,1)" and "None" can't be parsed into an integer).
      
      Finally, the comment about "Rust seems to experience an internal error" is only
      true in debug mode, where u32s are bounds-checked at runtime.  In release mode,
      code expressing the equivalent of this test will error with
      `Err(ProtoverError::Unparseable)` because 4294967295 is too large.
      f769edd1
    • Isis Lovecruft's avatar
      protover: Change protover_all_supported() to return only unsupported. · ad369313
      Isis Lovecruft authored
      Previously, if "Link=1-5" was supported, and you asked protover_all_supported()
      (or protover::all_supported() in Rust) if it supported "Link=3-999", the C
      version would return "Link=3-999" and the Rust would return "Link=6-999".  These
      both behave the same now, i.e. both return "Link=6-999".
      ad369313
    • Isis Lovecruft's avatar
      rust: Refactor protover::compute_for_old_tor(). · cd28b4c7
      Isis Lovecruft authored
      During code review and discussion with Chelsea Komlo, she pointed out
      that protover::compute_for_old_tor() was a public function whose
      return type was `&'static CStr`.  We both agree that C-like parts of
      APIs should:
      
      1. not be exposed publicly (to other Rust crates),
      2. only be called in the appropriate FFI code,
      3. not expose types which are meant for FFI code (e.g. `*mut char`,
         `CString`, `*const c_int`, etc.) to the pure-Rust code of other
         crates.
      4. FFI code (e.g. things in `ffi.rs` modules) should _never_ be called
         from pure-Rust, not even from other modules in its own crate
         (i.e. do not call `protover::ffi::*` from anywhere in
         `protover::protoset::*`, etc).
      
      With that in mind, this commit makes the following changes:
      
       * CHANGE `protover::compute_for_old_tor()` to be
         visible only at the `pub(crate)` level.
       * RENAME `protover::compute_for_old_tor()` to
         `protover::compute_for_old_tor_cstr()` to reflect the last change.
       * ADD a new `protover::compute_for_old_tor()` function wrapper which
         is public and intended for other Rust code to use, which returns a
         `&str`.
      cd28b4c7
    • Isis Lovecruft's avatar
      rust: Refactor Rust implementation of protover_is_supported_here(). · fd127bfb
      Isis Lovecruft authored
      It was changed to take borrows instead of taking ownership.
      
       * REFACTOR `protover::ffi::protover_is_supported_here()` to use changed method
         signature on `protover::is_supported_here()`.
      fd127bfb
    • Isis Lovecruft's avatar
      rust: Refactor Rust impl of protover_compute_vote(). · 32638ed4
      Isis Lovecruft authored
      This includes a subtle difference in behaviour to the previous Rust
      implementation, where, for each vote that we're computing over, if a single one
      fails to parse, we skip it.  This now matches the current behaviour in the C
      implementation.
      
       * REFACTOR `protover::ffi::protover_compute_vote()` to use
         new types and methods.
      32638ed4
    • Isis Lovecruft's avatar
      rust: Refactor Rust impl of protover_list_supports_protocol_or_later(). · 269053a3
      Isis Lovecruft authored
      This includes a subtle difference in behaviour, as in 4258f1e18, where we return
      (matching the C impl's return behaviour) earlier than before if parsing failed,
      saving us computation in parsing the versions into a
      protover::protoset::ProtoSet.
      
       * REFACTOR `protover::ffi::protover_list_supports_protocol_or_later()` to use
         new types and methods.
      269053a3
    • Isis Lovecruft's avatar
      rust: Refactor Rust impl of protover_list_supports_protocol(). · 63eeda89
      Isis Lovecruft authored
      This includes a subtle difference in behaviour, as in 4258f1e18, where we return
      (matching the C impl's return behaviour) earlier than before if parsing failed,
      saving us computation in parsing the versions into a
      protover::protoset::ProtoSet.
      
       * REFACTOR `protover::ffi::protover_list_supports_protocol()` to use new types
         and methods.
      63eeda89
    • Isis Lovecruft's avatar
      rust: Refactor Rust impl of protover_all_supported(). · c7bcca02
      Isis Lovecruft authored
      This includes differences in behaviour to before, which should now more closely
      match the C version:
      
       - If parsing a protover `char*` from C, and the string is not parseable, this
         function will return 1 early, which matches the C behaviour when protocols
         are unparseable.  Previously, we would parse it and its version numbers
         simultaneously, i.e. there was no fail early option, causing us to spend more
         time unnecessarily parsing versions.
      
       * REFACTOR `protover::ffi::protover_all_supported()` to use new types and
         methods.
      c7bcca02
    • Isis Lovecruft's avatar
      rust: Refactor protover tests with new methods; note altered behaviours. · 493e5652
      Isis Lovecruft authored
      Previously, the rust implementation of protover considered an empty string to be
      a valid ProtoEntry, while the C version did not (it must have a "=" character).
      Other differences include that unknown protocols must now be parsed as
      `protover::UnknownProtocol`s, and hence their entries as
      `protover::UnvalidatedProtoEntry`s, whereas before (nearly) all protoentries
      could be parsed regardless of how erroneous they might be considered by the C
      version.
      
      My apologies for this somewhat messy and difficult to read commit, if any part
      is frustrating to the reviewer, please feel free to ask me to split this into
      smaller changes (possibly hard to do, since so much changed), or ask me to
      comment on a specific line/change and clarify how/when the behaviours differ.
      
      The tests here should more closely match the behaviours exhibited by the C
      implementation, but I do not yet personally guarantee they match precisely.
      
       * REFACTOR unittests in protover::protover.
       * ADD new integration tests for previously untested behaviour.
       * FIXES part of #24031: https://bugs.torproject.org/24031.
      493e5652
    • Isis Lovecruft's avatar
      rust: Refactor protover::is_supported_here(). · 35b86a12
      Isis Lovecruft authored
      This changes `protover::is_supported_here()` to be aware of new datatypes
      (e.g. don't call `.0` on things which are no longer tuple structs) and also
      changes the method signature to take borrows, making it faster, threadable, and
      easier to read (i.e. the caller can know from reading the function signature
      that the function won't mutate values passed into it).
      
       * CHANGE the `protover::is_supported_here()` function to take borrows.
       * REFACTOR the `protover::is_supported_here()` function to be aware of new
         datatypes.
       * FIXES part of #24031: https://bugs.torproject.org/24031
      35b86a12
    • Isis Lovecruft's avatar
      rust: Add new ProtoverVote type and refactor functions to methods. · 2eb1b7f2
      Isis Lovecruft authored
      This adds a new type for votes upon `protover::ProtoEntry`s (technically, on
      `protover::UnvalidatedProtoEntry`s, because the C code does not validate based
      upon currently known protocols when voting, in order to maintain
      future-compatibility), and converts several functions which would have operated
      on this datatype into methods for ease-of-use and readability.
      
      This also fixes a behavioural differentce to the C version of
      protover_compute_vote().  The C version of protover_compute_vote() calls
      expand_protocol_list() which checks if there would be too many subprotocols *or*
      expanded individual version numbers, i.e. more than MAX_PROTOCOLS_TO_EXPAND, and
      does this *per vote* (but only in compute_vote(), everywhere else in the C seems
      to only care about the number of subprotocols, not the number of individual
      versions).  We need to match its behaviour in Rust and ensure we're not allowing
      more than it would to get the votes to match.
      
       * ADD new `protover::ProtoverVote` datatype.
       * REMOVE the `protover::compute_vote()` function and refactor it into an
         equivalent-in-behaviour albeit more memory-efficient voting algorithm based
         on the new underlying `protover::protoset::ProtoSet` datatype, as
         `ProtoverVote::compute()`.
       * REMOVE the `protover::write_vote_to_string()` function, since this
         functionality is now generated by the impl_to_string_for_proto_entry!() macro
         for both `ProtoEntry` and `UnvalidatedProtoEntry` (the latter of which is the
         correct type to return from a voting protocol instance, since the entity
         voting may not know of all protocols being voted upon or known about by other
         voting parties).
       * FIXES part of #24031: https://bugs.torproject.org/24031
      
      rust: Fix a difference in compute_vote() behaviour to C version.
      2eb1b7f2
    • Isis Lovecruft's avatar
      rust: Add macro for `impl ToString for {Unvalidated}ProtoEntry`. · fa15ea10
      Isis Lovecruft authored
      This implements conversions from either a ProtoEntry or an UnvalidatedProtoEntry
      into a String, for use in replacing such functions as
      `protover::write_vote_to_string()`.
      
       * ADD macro for implementing ToString trait for ProtoEntry and
         UnvalidatedProtoEntry.
       * FIXES part of #24031: https://bugs.torproject.org/24031
      fa15ea10
    • Isis Lovecruft's avatar
      rust: Add new protover::UnvalidatedProtoEntry type. · 3c47d31e
      Isis Lovecruft authored
      This adds a new protover::UnvalidatedProtoEntry type, which is the
      UnknownProtocol variant of a ProtoEntry, and refactors several functions which
      should operate on this type into methods.
      
      This also fixes what was previously another difference to the C implementation:
      if you asked the C version of protovet_compute_vote() to compute a single vote
      containing "Fribble=", it would return NULL.  However, the Rust version would
      return "Fribble=" since it didn't check if the versions were empty before
      constructing the string of differences.  ("Fribble=" is technically a valid
      protover string.)  This is now fixed, and the Rust version in that case will,
      analogous to (although safer than) C returning a NULL, return None.
      
       * REMOVE internal `contains_only_supported_protocols()` function.
       * REMOVE `all_supported()` function and refactor it into
         `UnvalidatedProtoEntry::all_supported()`.
       * REMOVE `parse_protocols_from_string_with_no_validation()` and
         refactor it into the more rusty implementation of
         `impl FromStr for UnvalidatedProtoEntry`.
       * REMOVE `protover_string_supports_protocol()` and refactor it into
         `UnvalidatedProtoEntry::supports_protocol()`.
       * REMOVE `protover_string_supports_protocol_or_later()` and refactor
         it into `UnvalidatedProtoEntry::supports_protocol_or_later()`.
       * FIXES part of #24031: https://bugs.torproject.org/24031
      
      rust: Fix another C/Rust different in compute_vote().
      
      This fixes the unittest from the prior commit by checking if the versions are
      empty before adding a protocol to a vote.
      3c47d31e
    • Isis Lovecruft's avatar
      rust: Add new protover::ProtoEntry type which uses new datatypes. · 54c96433
      Isis Lovecruft authored
      This replaces the `protover::SupportedProtocols` (why would you have a type just
      for things which are supported?) with a new, more generic type,
      `protover::ProtoEntry`, which utilises the new, more memory-efficient datatype
      in protover::protoset.
      
       * REMOVE `get_supported_protocols()` and `SupportedProtocols::tor_supported()`
         (since they were never used separately) and collapse their functionality into
         a single `ProtoEntry::supported()` method.
       * REMOVE `SupportedProtocols::from_proto_entries()` and reimplement its
         functionality as the more rusty `impl FromStr for ProtoEntry`.
       * REMOVE `get_proto_and_vers()` function and refactor it into the more rusty
         `impl FromStr for ProtoEntry`.
       * FIXES part of #24031: https://bugs.torproject.org/24031
      54c96433
    • Isis Lovecruft's avatar
      rust: Add new protover::UnknownProtocol type. · 60daaa68
      Isis Lovecruft authored
       * ADD new type, protover::UnknownProtocol, so that we have greater type safety
         and our protover functionality which works with unsanitised protocol names is
         more clearly demarcated.
       * REFACTOR protover::Proto, renaming it protover::Protocol to mirror the new
         protover::UnknownProtocol type name.
       * ADD a utility conversion of `impl From<Protocol> for UnknownProtocol` so that
         we can easily with known protocols and unknown protocols simultaneously
         (e.g. doing comparisons, checking their version numbers), while not allowing
         UnknownProtocols to be accidentally used in functions which should only take
         Protocols.
       * FIXES part of #24031: https://bugs.torproject.org/24031
      60daaa68
    • Isis Lovecruft's avatar
      rust: Implement more memory-efficient protover datatype. · e6625113
      Isis Lovecruft authored
       * ADD new protover::protoset module.
       * ADD new protover::protoset::ProtoSet class for holding protover versions.
       * REMOVE protover::Versions type implementation and its method
         `from_version_string()`, and instead implement this behaviour in a more
         rust-like manner as `impl FromStr for ProtoSet`.
       * MOVE the `find_range()` utility function from protover::protover to
         protover::protoset since it's only used internally in the
         implementation of ProtoSet.
       * REMOVE the `contract_protocol_list()` function from protover::protover and
         instead refactor it (reusing nearly the entire thing, with minor superficial,
         i.e. non-behavioural, changes) into a more rusty
         `impl ToString for ProtoSet`.
       * REMOVE the `expand_version_range()` function from protover::protover and
         instead refactor it into a more rusty implementation of
         `impl Into<Vec<Version>> for ProtoSet` using the new error types in
         protover::errors.
       * FIXES part of #24031: https://bugs.torproject.org/24031.
      e6625113
    • Isis Lovecruft's avatar
      rust: Implement error types for Rust protover implementation. · 88204f91
      Isis Lovecruft authored
      This will allow us to do actual error handling intra-crate in a more
      rusty manner, e.g. propogating errors in match statements, conversion
      between error types, logging messages, etc.
      
       * FIXES part of #24031: https://bugs.torproject.org/24031.
      88204f91
    • Isis Lovecruft's avatar
      rust: Fix ProtoSet and ProtoEntry to use the same DoS limits as C. · f2daf827
      Isis Lovecruft authored
      Previously, the limit for MAX_PROTOCOLS_TO_EXPAND was actually being applied
      in Rust to the maximum number of version (total, for all subprotocols).
      Whereas in C, it was being applied to the number of subprotocols that were
      allowed.  This changes the Rust to match C's behaviour.
      f2daf827
    • Isis Lovecruft's avatar
      rust: Port all C protover_all_supported tests to Rust. · 6eea0dc5
      Isis Lovecruft authored
      The behaviours still do not match, unsurprisingly, but now we know where a
      primary difference is: the Rust is validating version ranges more than the C,
      so in the C it's possible to call protover_all_supported on a ridiculous
      version range like "Sleen=0-4294967294" because the C uses
      MAX_PROTOCOLS_TO_EXPAND to count the number of *subprotocols* whereas the Rust
      uses it to count the total number of *versions* of all subprotocols.
      6eea0dc5
    • Isis Lovecruft's avatar
      tests: Run all existing protover tests in both languages. · 527a2398
      Isis Lovecruft authored
      There's now no difference in these tests w.r.t. the C or Rust: both
      fail miserably (well, Rust fails with nice descriptive errors, and C
      gives you a traceback, because, well, C).
      527a2398
    • Isis Lovecruft's avatar
      tests: Make inline comments in test_protover.c more accurate. · 22c65a0e
      Isis Lovecruft authored
      The DoS potential is slightly higher in C now due to some differences to the
      Rust code, see the C_RUST_DIFFERS tags in src/rust/protover/tests/protover.rs.
      
      Also, the comment about "failing at the splitting stage" in Rust wasn't true,
      since when we split, we ignore empty chunks (e.g. "1--1" parses into
      "(1,None),(None,1)" and "None" can't be parsed into an integer).
      
      Finally, the comment about "Rust seems to experience an internal error" is only
      true in debug mode, where u32s are bounds-checked at runtime.  In release mode,
      code expressing the equivalent of this test will error with
      `Err(ProtoverError::Unparseable)` because 4294967295 is too large.
      22c65a0e
    • Isis Lovecruft's avatar
      protover: Change protover_all_supported() to return only unsupported. · 6e353664
      Isis Lovecruft authored
      Previously, if "Link=1-5" was supported, and you asked protover_all_supported()
      (or protover::all_supported() in Rust) if it supported "Link=3-999", the C
      version would return "Link=3-999" and the Rust would return "Link=6-999".  These
      both behave the same now, i.e. both return "Link=6-999".
      6e353664
    • Isis Lovecruft's avatar
      rust: Refactor protover::compute_for_old_tor(). · fc2a42cc
      Isis Lovecruft authored
      During code review and discussion with Chelsea Komlo, she pointed out
      that protover::compute_for_old_tor() was a public function whose
      return type was `&'static CStr`.  We both agree that C-like parts of
      APIs should:
      
      1. not be exposed publicly (to other Rust crates),
      2. only be called in the appropriate FFI code,
      3. not expose types which are meant for FFI code (e.g. `*mut char`,
         `CString`, `*const c_int`, etc.) to the pure-Rust code of other
         crates.
      4. FFI code (e.g. things in `ffi.rs` modules) should _never_ be called
         from pure-Rust, not even from other modules in its own crate
         (i.e. do not call `protover::ffi::*` from anywhere in
         `protover::protoset::*`, etc).
      
      With that in mind, this commit makes the following changes:
      
       * CHANGE `protover::compute_for_old_tor()` to be
         visible only at the `pub(crate)` level.
       * RENAME `protover::compute_for_old_tor()` to
         `protover::compute_for_old_tor_cstr()` to reflect the last change.
       * ADD a new `protover::compute_for_old_tor()` function wrapper which
         is public and intended for other Rust code to use, which returns a
         `&str`.
      fc2a42cc
    • Isis Lovecruft's avatar
      rust: Refactor Rust implementation of protover_is_supported_here(). · 9766d53c
      Isis Lovecruft authored
      It was changed to take borrows instead of taking ownership.
      
       * REFACTOR `protover::ffi::protover_is_supported_here()` to use changed method
         signature on `protover::is_supported_here()`.
      9766d53c
    • Isis Lovecruft's avatar
      rust: Refactor Rust impl of protover_compute_vote(). · 6f252e09
      Isis Lovecruft authored
      This includes a subtle difference in behaviour to the previous Rust
      implementation, where, for each vote that we're computing over, if a single one
      fails to parse, we skip it.  This now matches the current behaviour in the C
      implementation.
      
       * REFACTOR `protover::ffi::protover_compute_vote()` to use
         new types and methods.
      6f252e09
    • Isis Lovecruft's avatar
      rust: Refactor Rust impl of protover_list_supports_protocol_or_later(). · 0a5494b8
      Isis Lovecruft authored
      This includes a subtle difference in behaviour, as in 4258f1e18, where we return
      (matching the C impl's return behaviour) earlier than before if parsing failed,
      saving us computation in parsing the versions into a
      protover::protoset::ProtoSet.
      
       * REFACTOR `protover::ffi::protover_list_supports_protocol_or_later()` to use
         new types and methods.
      0a5494b8
    • Isis Lovecruft's avatar
      rust: Refactor Rust impl of protover_list_supports_protocol(). · 52c3ea50
      Isis Lovecruft authored
      This includes a subtle difference in behaviour, as in 4258f1e18, where we return
      (matching the C impl's return behaviour) earlier than before if parsing failed,
      saving us computation in parsing the versions into a
      protover::protoset::ProtoSet.
      
       * REFACTOR `protover::ffi::protover_list_supports_protocol()` to use new types
         and methods.
      52c3ea50
    • Isis Lovecruft's avatar
      rust: Refactor Rust impl of protover_all_supported(). · 2f3a7376
      Isis Lovecruft authored
      This includes differences in behaviour to before, which should now more closely
      match the C version:
      
       - If parsing a protover `char*` from C, and the string is not parseable, this
         function will return 1 early, which matches the C behaviour when protocols
         are unparseable.  Previously, we would parse it and its version numbers
         simultaneously, i.e. there was no fail early option, causing us to spend more
         time unnecessarily parsing versions.
      
       * REFACTOR `protover::ffi::protover_all_supported()` to use new types and
         methods.
      2f3a7376
    • Isis Lovecruft's avatar
      rust: Refactor protover tests with new methods; note altered behaviours. · 15e59a1f
      Isis Lovecruft authored
      Previously, the rust implementation of protover considered an empty string to be
      a valid ProtoEntry, while the C version did not (it must have a "=" character).
      Other differences include that unknown protocols must now be parsed as
      `protover::UnknownProtocol`s, and hence their entries as
      `protover::UnvalidatedProtoEntry`s, whereas before (nearly) all protoentries
      could be parsed regardless of how erroneous they might be considered by the C
      version.
      
      My apologies for this somewhat messy and difficult to read commit, if any part
      is frustrating to the reviewer, please feel free to ask me to split this into
      smaller changes (possibly hard to do, since so much changed), or ask me to
      comment on a specific line/change and clarify how/when the behaviours differ.
      
      The tests here should more closely match the behaviours exhibited by the C
      implementation, but I do not yet personally guarantee they match precisely.
      
       * REFACTOR unittests in protover::protover.
       * ADD new integration tests for previously untested behaviour.
       * FIXES part of #24031: https://bugs.torproject.org/24031.
      15e59a1f