24902 fix breaks on clang

https://travis-ci.org/torproject/tor/jobs/335395212#L1716

src/or/dos.c:277:40: error: implicit conversion loses integer precision: 'long'
      to 'uint32_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
  num_token = elapsed_time_last_refill * circuit_rate;
            ~ ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~

changed milestone to %Tor: 0.3.3.x-final in legacy/trac

added actualpoints::0.2 in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.3.3.x-final in Legacy / Trac parent::24902 in Legacy / Trac points::0.2 in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tor-dos in Legacy / Trac type::defect in Legacy / Trac labels

Trac:
Parent: N/A to legacy/trac#24902 (moved)

Let's target master with this ticket, and do the backport in legacy/trac#24902 (moved).

Also, the round here is redundant and can be removed:

src/test/test_dos.c:179:19: error: implicit conversion loses integer precision:
      'long' to 'int' [-Werror,-Wshorten-64-to-32]
  int circ_rate = tor_lround(get_circuit_rate_per_second());
      ~~~~~~~~~   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

See my branch bug25094 for fixes to clang errors, including better overflow checks in cc_stats_refill_bucket, and some more unit tests.

Trac:
Status: new to needs_review
Milestone: Tor: 0.2.9.x-final to Tor: 0.3.3.x-final

teor: while you're messing with that function, what would you think of adding an "if elapsed time is zero, just return" line in there? That way we only do the log_debug when we actually refilled something.

I think we should return if we don't do anything. And we should return if the old time is zero, and the current time is zero. Then I can turn those unit tests back on.

I added another commit that skips the entire function if the current time is equal to the last refill time.

This makes the time == 0 edge case nicer.

The behaviour was:

• new clients get a full refill on every cell as long as time == 0
• existing clients get a full refill on every cell as long as time == 0

Now it is:

• new clients don't get refills as long as time == 0
• new clients get one full refill as soon as time >= 1
• existing clients get one normal refill if they have any cells when time == 0
• existing clients get one full refill as soon as time >= 1, if they had a refill when time == 0

And I think that's about as much time as it's worth spending on this edge case.

Trac:
Actualpoints: N/A to 0.2
Points: N/A to 0.2

I've taken teor patch and fixed very minor thing (mostly syntax) and made the circuit_rate to be a uint64_t instead of casting it. The getter returns the uin32_t value so we are good in conversion there. Just one less cast.

I've re-arranged the commit to have 3 of them, (1) teor's test fix, (2) refill over/underflow checks, (3) add unit tests.

gcc and clang seems happy here.

Commits are in branch: ticket24902_029_05 so it is easy for nick to merge it forward into master. Let me know if you want me to move them to the _033_02 branch or just plain latest master.

Trac:
Keywords: N/A deleted, tor-dos added
Status: needs_review to merge_ready

Merges cleanly to master, and passes tests for me. Merging to master.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

Replying to dgoulet:

I've taken teor patch and fixed very minor thing (mostly syntax) and made the circuit_rate to be a uint64_t instead of casting it. The getter returns the uin32_t value so we are good in conversion there. Just one less cast.

This works, but there are no unit tests that make sure it does. (We could add them if we liked.)

Also, there is no comment that explains why we return a uint32_t as a uint64_t.

I don't think we need to fix either of these things. If anyone wants to, we can do it in master going forward.

closed

changed time estimate to 1h 36m

added 1h 36m of time spent

moved from legacy/trac#25094 (moved)

added Bug label and removed 1 deleted label

added 1 deleted label and removed 1 deleted label

added DoS label and removed 1 deleted label