#25202 closed defect (fixed)

Check the calculations in cc_stats_refill_bucket using non fatal assertions

Reported by: teor Owned by: dgoulet
Priority: Low Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dos, review-group-32, 033-must
Cc: Actual Points:
Parent ID: #24902 Points: 0.1
Reviewer: nickm Sponsor:

Description (last modified by teor)

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!

Child Tickets

Change History (12)

comment:1 Changed 10 months ago by teor

Description: modified (diff)
Status: newneeds_review

Spacing, and someone else might as well look at this for review

comment:2 Changed 10 months ago by teor

Description: modified (diff)

Spacing and asterisks

comment:3 Changed 10 months ago by dgoulet

Priority: MediumLow

I'm ok with both asserts, no strong opinion on this. I don't think the first assert is really useful for "safety" as it is more about safety for "future code change" which I hope also the unit tests will catch anything on that front.

The second assert is interesting as it reinforces the fact that the function can ONLY increment the bucket or set it to the allowed burst that could be smaller than the current count.

Both cases, I see this as a defense in depth.

comment:4 Changed 10 months ago by nickm

Keywords: review-group-32 added

comment:5 Changed 10 months ago by dgoulet

Reviewer: dgoulet

comment:6 Changed 10 months ago by dgoulet

Owner: set to dgoulet
Parent ID: #24902
Reviewer: dgoulet
Status: needs_reviewaccepted

Oh hey, I still like this change but I would like to hold off on it until we get #24902 backported else we keep piling up things on ticket24902_029_05 which is getting big!

So I'll put this back in Assigned, unparent it and and flag it for backport. I'll make a branch based on 029 once the above is merged.

comment:7 Changed 10 months ago by dgoulet

Keywords: 029-backport 031-backport 032-backport added

comment:8 Changed 10 months ago by dgoulet

Keywords: 033-must added

comment:9 Changed 10 months ago by dgoulet

Keywords: 029-backport 031-backport 032-backport removed
Parent ID: #24902

Scratch the above as a discussion with nickm, we'll pile on 24902 so we can put it in 033 before any backport.

comment:10 Changed 10 months ago by dgoulet

Status: acceptedneeds_review

See branch ticket25202_029_01.

Based on ticket24902_029_05 branch. Once ACK, I'll merge this into that parent branch for the 029 backport.

comment:11 Changed 10 months ago by nickm

Status: needs_reviewmerge_ready

lgtm, if you believe that these assertions can't fail.

comment:12 Changed 10 months ago by dgoulet

Resolution: fixed
Reviewer: nickm
Status: merge_readyclosed

Merged into #24902.

Note: See TracTickets for help on using tickets.