#25669: Privcount: blinding and encryption should be finished up -------------------------------------------------+------------------------- Reporter: nickm | Owner: (none) Type: enhancement | Status: | needs_revision Priority: Medium | Milestone: Tor: | 0.3.5.x-final Component: Core Tor/Tor | Version: Severity: Normal | Resolution: Keywords: privcount, 035-roadmap-master, 035 | Actual Points: -triaged-in-20180711, rust | Parent ID: #22898 | Points: Reviewer: | Sponsor: | SponsorV-can -------------------------------------------------+-------------------------
Comment (by chelseakomlo): Overall, this is looking good. This review is purely a Rust convention and code quality review, I didn't actually look deeply at how logic is implemented. Doctests and more unit tests would help in being able to do a deeper review for logic though. As a more meta point, there is a lot of good and very detailed information in our Rust coding standards: tor/docs/HACKING/CodingStandardsRust.md. I would recommend taking a look, particularly for safety if/when this code integrates with tor C code. Let me know if you want to talk over any of this on irc/etc. == Types - In several places, it looks like `usize` is being used interchangeably for integer types (such as `test_combination`). I would recommend doing a review to ensure if using explicit fixed-size representations would be safer in these cases, as usize is of pointer size, so its size changes depending on the architecture. https://doc.rust- lang.org/std/primitive.usize.html. == Documentation - Doctests should probably be added to all public functions- this is helpful for both documenting the code and also verifying that the documentation is valid. - Readme needs to be updated: https://github.com/nmathewson/privcount_shamir - Per our Rust coding standards (in tor/docs/HACKING/CodingStandardsRust.md, `#[deny(missing_docs)]` must be included == External Dependencies - I see that this crate depends on several external crates. `rust-crypto` states that it doesn't have strong security guarantees- is there something else that we should be using? https://crates.io/crates/rust-crypto. Should we have an auditing process for when we choose to import/use new external crates? == Rust code conventions - Snake case in the function name: https://github.com/nmathewson/privcount_shamir/blob/master/rust/src/server.rs#L69 == Integration - How will this code be executed? Will something in core Tor call into these public functions? If so, where is the FFI layer? - Should any logging be added to this module? - Will this code call into Tor's C code at all? It doesn't currently but I was curious if there are plans for this in the case of the code moving into the core tor codebase. == Safety - Ensure that if this code is being called from C, there aren't unexpected places that would lead to UB. For example, everything needs to be explicitly handled in the case of errors: https://github.com/nmathewson/privcount_shamir/blob/master/rust/src/encrypt.rs#L122 as one example, but I see `assert!` in several places as well as `assert_eq!`, etc: https://github.com/nmathewson/privcount_shamir/blob/f679aa90ef3ce0d264c50f7e010b2b8022b96c7c/rust/src/shamir.rs#L90 - Are there bounds checks that should happen in these places? https://github.com/nmathewson/privcount_shamir/blob/f679aa90ef3ce0d264c50f7e010b2b8022b96c7c/rust/src/shamir.rs#L50 and https://github.com/nmathewson/privcount_shamir/blob/34db770b2bed5ee7217bc01a530e39b1a90bad17/rust/src/data.rs#L87 - Handle error cases and don't call unwrap directly. https://github.com/nmathewson/privcount_shamir/blob/fe039e5600c25b99acd4bd23b0e609874789fd79/rust/src/client.rs#L25 and https://github.com/nmathewson/privcount_shamir/blob/fe039e5600c25b99acd4bd23b0e609874789fd79/rust/src/client.rs#L100 for example. == Testing === Unit tests - I see a couple modules without unit tests (client, data). Per our coding standards, all code must be unit and integration tests. For extremely basic functions, doctests are likely sufficient (such as to string methods) but this should be in only in extremely simple cases and not the default. - Ensure test names are descriptive of what they are testing, or add comments: https://github.com/nmathewson/privcount_shamir/blob/f679aa90ef3ce0d264c50f7e010b2b8022b96c7c/rust/src/shamir.rs#L131 - Remove print statements in tests :) https://github.com/nmathewson/privcount_shamir/blob/f679aa90ef3ce0d264c50f7e010b2b8022b96c7c/rust/src/shamir.rs#L138 === Integration tests - Are there any extreme cases that should be tested here? I.e, high values, low values, large values, etc. - Can you add a more descriptive name to each test case? I.e, if those are valid test cases, maybe specify that in the test name. - Try to pick explicit naming where possible- `n_trs` isn't totally clear: https://github.com/nmathewson/privcount_shamir/blob/master/rust/tests/basic_integration.rs#L26 -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25669#comment:15> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs