[tor-bugs] #25202 [Core Tor/Tor]: Check the calculations in cc_stats_refill_bucket using non fatal assertions

[Tor Bug Tracker & Wiki]

#25202: Check the calculations in cc_stats_refill_bucket using non fatal assertions
--------------------------+------------------------------------
 Reporter:  teor          |          Owner:  (none)
     Type:  defect        |         Status:  needs_review
 Priority:  Medium        |      Milestone:  Tor: 0.3.3.x-final
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:  tor-dos       |  Actual Points:
Parent ID:  #24902        |         Points:  0.1
 Reviewer:                |        Sponsor:
--------------------------+------------------------------------

Old description:

> In #25128, we removed an incorrect non-fatal assertion in
> cc_stats_refill_bucket() to silence a warning:
> {{{
>   /* This function is not allowed to make the bucket count smaller */
>   tor_assert_nonfatal(new_circuit_bucket_count >= stats->circuit_bucket);
> }}}
>
> But we could have fixed the check instead, and added another check:
> {{{
>   /* This function is not allowed to make the bucket count larger than
> the burst value */
>   tor_assert_nonfatal(new_circuit_bucket_count <= dos_cc_circuit_burst);
>   /* This function is not allowed to make the bucket count smaller,
> unless it is
>       decreasing it to a newly configured, lower burst value. We allow
> the bucket to
>       stay the same size, in case the circuit rate is zero. */
>   tor_assert_nonfatal(new_circuit_bucket_count >= stats->circuit_bucket
> ||
>       new_circuit_bucket_count == dos_cc_circuit_burst);
> }}}
>
> We could be even more clever, and skip parts of the function if the rate
> is zero. That's probably unnecessary. I'll think about it.
>
> I should get a chance to turn this into a proper branch over the next
> week or so. If someone else wants to do it before then, go for it!

New description:

 In #25128, we removed an incorrect non-fatal assertion in
 cc_stats_refill_bucket() to silence a warning:
 {{{
   /* This function is not allowed to make the bucket count smaller */
   tor_assert_nonfatal(new_circuit_bucket_count >= stats->circuit_bucket);
 }}}

 But we could have fixed the check instead, and added another check:
 {{{
   /* This function is not allowed to make the bucket count larger than the
 burst value */
   tor_assert_nonfatal(new_circuit_bucket_count <= dos_cc_circuit_burst);
   /* This function is not allowed to make the bucket count smaller, unless
 it is
    * decreasing it to a newly configured, lower burst value. We allow the
 bucket to
    * stay the same size, in case the circuit rate is zero. */
   tor_assert_nonfatal(new_circuit_bucket_count >= stats->circuit_bucket ||
     new_circuit_bucket_count == dos_cc_circuit_burst);
 }}}

 We could be even more clever, and skip parts of the function if the rate
 is zero. That's probably unnecessary. I'll think about it.

 I should get a chance to turn this into a proper branch over the next week
 or so. If someone else wants to do it before then, go for it!

--

Comment (by teor):

 Spacing and asterisks

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