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

Re: [tor-bugs] #27165 [Core Tor/Tor]: CID 1438153 unlikely overflow in predicted_ports_prediction_time_remaining()

#27165: CID 1438153 unlikely overflow in
predicted_ports_prediction_time_remaining()
--------------------------+----------------------------------
 Reporter:  catalyst      |          Owner:  (none)
     Type:  defect        |         Status:  needs_review
 Priority:  Medium        |      Milestone:  Tor: unspecified
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:  coverity      |  Actual Points:
Parent ID:                |         Points:
 Reviewer:                |        Sponsor:
--------------------------+----------------------------------

Comment (by teor):

 The time_t changes are a good idea.

 But I suggest you do the cast back to int once, in
 predicted_ports_prediction_time_remaining().

 And the new overflow check:
 {{{
  if (BUG((prediction_timeout - idle_delta) > TIME_MAX)) {
 }}}

 Is wrong, because `(time_t)x > TIME_MAX` is always false.

 Instead, use the correct test:
 {{{
   /* We check that idle_delta is less than or equal to prediction_timeout,
 so any overflow is a bug */
   if (BUG((prediction_timeout - idle_delta) > INT_MAX)) {
      return INT_MAX;
   }
 }}}

 But this check may also invoke undefined behaviour, if prediction_timeout
 is TIME_T_MAX and idle_delta is negative. idle_delta can be negative if
 now is negative (or now - last_prediction_add_time overflows, because now
 is TIME_T_MAX and last_prediction_add_time is negative).

 But if we try to algebraically rearrange it so it doesn't invoke undefined
 behaviour, we end up causing undefined behaviour when TIME_T and int are
 the same size:
 {{{
   /* We check that idle_delta is less than or equal to prediction_timeout,
 so any overflow is a bug */
   if (BUG(prediction_timeout > (INT_MAX + idle_delta))) {
      return INT_MAX;
   }
 }}}

 So maybe we want this arrangement:
 {{{
   /* We check that idle_delta is less than or equal to prediction_timeout,
 so any overflow is a bug */
   if (BUG(-idle_delta > (INT_MAX - prediction_timeout))) {
      return INT_MAX;
   }
 }}}

 But that's not undefined-behaviour free, either.

 Maybe we should think carefully about what we're trying to do here?
 So I suggest that we

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27165#comment:2>
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