[tor-bugs] #7801 [Tor]: Our one use of tor_weak_random() is subtly wrong

[Tor Bug Tracker & Wiki]

#7801: Our one use of tor_weak_random() is subtly wrong
-----------------------+----------------------------------------------------
 Reporter:  nickm      |          Owner:                    
     Type:  defect     |         Status:  new               
 Priority:  minor      |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor        |        Version:                    
 Keywords:  tor-relay  |         Parent:                    
   Points:             |   Actualpoints:                    
-----------------------+----------------------------------------------------

Comment(by nickm):

 Yup.  Note that the code '''isn't'''
 {{{
     int num_streams = 0;
     for (conn = first_conn; conn; conn = conn->next_stream) {
       num_streams++;
       if ((tor_weak_random() % num_streams)==0) {
         chosen_stream = conn; break;
       }
     }
 }}}
 Instead, it was:
 {{{
     int num_streams = 0;
     for (conn = first_conn; conn; conn = conn->next_stream) {
       num_streams++;
       if ((tor_weak_random() % num_streams)==0)
         chosen_stream = conn;
       // no break here.
     }
 }}}

 So the first time through the loop, chosen_stream is always first_conn,
 but the second time through the loop, it becomes the second connection
 with P=1/2, and so on.  I've tried to make this even more explicit in the
 comment.

 And wrt the segfault, it can't happen because if first_conn is NULL,
 chosen_stream will be NULL.  So in the first loop (The one after "FY:I")
 the loop condition will be false immediately, since conn will be set to
 NULL.  And in the second loop (the one after "correct one:"),
 conn=first_conn will set conn to NULL, and conn!=chosen_stream will
 compare conn with NULL, so again the loop body is never executed.

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