[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #19043 [Core Tor/Tor]: prop224: Implementation of ESTABLISH_INTRO cell

#19043: prop224: Implementation of ESTABLISH_INTRO cell
-------------------------------------------------+-------------------------
 Reporter:  alec.heif                            |          Owner:  asn
     Type:  enhancement                          |         Status:
                                                 |  merge_ready
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.0.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-hs, prop224, 6.s194,             |  Actual Points:
  TorCoreTeam201611, review-group-12             |
Parent ID:  #17241                               |         Points:  6
 Reviewer:  asn, dgoulet                         |        Sponsor:
                                                 |  SponsorR-must
-------------------------------------------------+-------------------------
Changes (by asn):

 * status:  needs_revision => merge_ready


Comment:

 Replying to [comment:36 chelseakomlo]:
 > All of the new modules look great!
 >
 > A couple minor points, feel free to take/leave any:
 >

 Thanks for the review. I force pushed the fixes to the same branch.

 > - There are some magic numbers in encode_establish_intro_cell_legacy-
 some of these could be extracted into defines

 Hmm, that function is (very) old code. I tried to leave it alone as much
 as possible.

 I'm inclined to suggest that any improvements to that old code should be
 done on a new ticket, so that reviewers of this ticket have to worry as
 little as possible about regressions.

 > - the_hs_circuitmap might be renamed for specificity- the_ is a bit
 unclear.

 Hmm, I could rename that to `global_hs_circuitmap` or something.

 That said, this silly `the_` prefix is used in multiple places in our
 codebase to name big file-level singletons (e.g. `the_nodelist`,
 `the_evdns_base`, etc.).

 Please let me know if you still feel like `global_hs_circuitmap` or
 something like that would be better, and I will just rename it.

 > - In the comment for hs_circuits_have_same_token we use introduction and
 rendezvous tokens, but in the function we use first_token and
 second_token. Maybe introduction/rendezvous naming can be consistently
 used.

 Hmm, that function is abstract on purpose. The HS circuitmap can carry
 both introduction and rendezvous tokens, but that function actually does
 not care about the token type. Do you think the function should be split
 into two functions handling each case separately or something?

 > - In commit f4db1ab5fcf3e48c6b4011e9f1cbae6db1aa8c5b it says that
 hs_received_establish_intro is the new entry point, but I think this is
 meant to be hs_intro_received_establish_intro

 Oops. Fixed that commit msg and force-pushed to the same branch name
 (naughty I know).

 > - In general, it can be useful to write multiple unit tests per function
 under test if there are branches in the code. For example, it looks like
 we just test one path for generate_establish_intro_cell, but it might be
 worth writing additional unit test to cover error cases.

 I wrote a unittest for one error case of that function and pushed it to
 the top of the branch. It even found a bug! Please check the top commit
 and if you like it I will squash it along with the rest of the branch so
 that it's clean for Nick's review.

 So the bug was there because I was not checking the retval of
 `crypto_hmac_sha3_256()` correctly... Not sure why we have `1` being the
 error retval in `crypto.c` instead of `-1`, perhaps I should address this
 in `crypto_hmac_sha3_256()`?

 > - encode_establish_intro_cell_legacy might be able to be unit tested
 directly. It looks like it is tested indirectly but we could have several
 unit tests to test the different branches of this directly.

 I think that old function is more unittestable now indeed. However, I'd
 suggest we make a new ticket for unittesting that legacy function.

 Putting this in `needs_review`, please let me know if it needs any other
 changes!!!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19043#comment:38>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs