Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#25094 closed defect (fixed)

24902 fix breaks on clang

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dos
Cc: Actual Points: 0.2
Parent ID: #24902 Points: 0.2
Reviewer: Sponsor:

Description

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;
            ~ ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~

Child Tickets

Change History (8)

comment:1 Changed 8 months ago by teor

Parent ID: #24902

comment:2 Changed 8 months ago by teor

Milestone: Tor: 0.2.9.x-finalTor: 0.3.3.x-final
Status: newneeds_review

Let's target master with this ticket, and do the backport in #24902.

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.

comment:3 Changed 8 months ago by arma

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.

comment:4 Changed 8 months ago by teor

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.

comment:5 Changed 8 months ago by teor

Actual Points: 0.2
Points: 0.2

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.

comment:6 Changed 8 months ago by dgoulet

Keywords: tor-dos added
Status: needs_reviewmerge_ready

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.

comment:7 Changed 8 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

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

comment:8 in reply to:  6 Changed 8 months ago by teor

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.

Note: See TracTickets for help on using tickets.