Prop 310 #1773
Prop 310 #1773
Conversation
Pull Request Test Coverage Report for Build 8778
|
Straight apply of suggestion from github feature Co-Authored-By: teor <teor@torproject.org>
src/feature/client/entrynodes.c
Outdated
| } else { | ||
| log_warn(LD_GUARD, "The state file seems to be into a status that could" | ||
| " yield to weird entry node selection. It might be better to delete" | ||
| " it"); |
Let's not suggest that people delete their state files. Instead let's warn, and set the sampled_idx to something. Leaving sampled_idx at zero seems like a worst-case idea, since it makes the guard get selected before any others.
idem as above
src/feature/client/entrynodes.c
Outdated
| @@ -3184,13 +3237,22 @@ entry_guards_load_guards_from_state(or_state_t *state, int set) | |||
| tor_assert(gs); | |||
| smartlist_add(gs->sampled_entry_guards, guard); | |||
| guard->in_selection = gs; | |||
| /* Recompute the next_sampled_id from the state */ | |||
| if (gs->next_sampled_idx <= guard->sampled_idx) { | |||
| gs->next_sampled_idx = ++guard->sampled_idx; | |||
Wait -- why are you incrementing guard->sampled_idx here? Don't you just want to say "guard->sampled_idx + 1"?
Wait -- yes. Now I wonder why a test did not catch it. Looking into it.
| @@ -1849,6 +1868,9 @@ make_guard_confirmed(guard_selection_t *gs, entry_guard_t *guard) | |||
|
|
|||
| guard->confirmed_idx = gs->next_confirmed_idx++; | |||
| smartlist_add(gs->confirmed_entry_guards, guard); | |||
| /** The confirmation ordering might not be the sample ording. We need to | |||
| * reorder */ | |||
Let's document this new requirement in the documentation for confirmed_entry_guards.
ok, done in struct guard_selection_t
Hmm, compare_guards_by_sampled_idx() only talks about sampled set but here we do it in the confirmed set. So perhaps we should update documentation?
| @@ -1771,7 +1773,7 @@ sample_reachable_filtered_entry_guards(guard_selection_t *gs, | |||
| flags, smartlist_len(reachable_filtered_sample)); | |||
|
|
|||
| if (smartlist_len(reachable_filtered_sample)) { | |||
| result = smartlist_choose(reachable_filtered_sample); | |||
| result = smartlist_get(reachable_filtered_sample, 0); | |||
Let's have a comment here that refers to proposal 310.
In particular, let's explain why this is safe, and why it won't just return the same guard over and over whether it's working or not.
Ok, done. I've also noticed that functions calling first_reachable_filtered_entry_guard needed documentation updates. Function select_filtered_guard_for_circuit documented to look into confirmed guards while excluding them; I believe the documentation was wrong but the code was ok (i.e., we had to exclude confirmed guards).
Hmm, I'd like to echo this. While prop310 is noted now, it's still not clear to me why the same guard wont be returned over and over. Perhaps some additional documentation or spec patch is in order here to increase comprehension?
|
I am not sure to get what's going on regarding the mem leak. It looks like some memory is leaking while parsing the config lines of the state file (the 8 lines of sampled_idx). This should be handled by config_free_lines, right? I don't see why this line is not freed. ================================================================= Direct leak of 17 byte(s) in 8 object(s) allocated from: Direct leak of 17 byte(s) in 8 object(s) allocated from: Direct leak of 17 byte(s) in 8 object(s) allocated from: |
… parts in auxiliary functions
| @@ -182,10 +182,10 @@ problem function-size /src/feature/client/addressmap.c:addressmap_rewrite() 109 | |||
| problem function-size /src/feature/client/bridges.c:rewrite_node_address_for_bridge() 125 | |||
| problem function-size /src/feature/client/circpathbias.c:pathbias_measure_close_rate() 108 | |||
| problem function-size /src/feature/client/dnsserv.c:evdns_server_callback() 153 | |||
| problem file-size /src/feature/client/entrynodes.c 3827 | |||
| problem file-size /src/feature/client/entrynodes.c 4000 | |||
Notes for the reviewer:
173 unchecked extra lines seems a bit high.
| problem file-size /src/feature/client/entrynodes.c 4000 | |
| problem file-size /src/feature/client/entrynodes.c 3900 |
| @@ -1771,7 +1773,7 @@ sample_reachable_filtered_entry_guards(guard_selection_t *gs, | |||
| flags, smartlist_len(reachable_filtered_sample)); | |||
|
|
|||
| if (smartlist_len(reachable_filtered_sample)) { | |||
| result = smartlist_choose(reachable_filtered_sample); | |||
| result = smartlist_get(reachable_filtered_sample, 0); | |||
Hmm, I'd like to echo this. While prop310 is noted now, it's still not clear to me why the same guard wont be returned over and over. Perhaps some additional documentation or spec patch is in order here to increase comprehension?
| @@ -1849,6 +1868,9 @@ make_guard_confirmed(guard_selection_t *gs, entry_guard_t *guard) | |||
|
|
|||
| guard->confirmed_idx = gs->next_confirmed_idx++; | |||
| smartlist_add(gs->confirmed_entry_guards, guard); | |||
| /** The confirmation ordering might not be the sample ording. We need to | |||
| * reorder */ | |||
Hmm, compare_guards_by_sampled_idx() only talks about sampled set but here we do it in the confirmed set. So perhaps we should update documentation?
| /** | ||
| * In what order was this guard sampled without replacement? Guards with | ||
| * lower indices appear earlier on the sampled list | ||
| */ |
I think we need a patch for guard-spec.txt to explain how this new field should work. Our guard spec is quite engineering oriented and basically explains how these fields tie together, so I think mentioning sampled_idx and next_sampled_idx there is important.
I can send a PR to the torspec repository (I guess guard-spec.txt is there)
Yes please!
Implements Proposal 310 issued from trac ticket 32088.
This patch also covers Nickm's bucket list from the ticket, i.e.:
The text was updated successfully, but these errors were encountered: