Opened 9 months ago

Last modified 6 months ago

#33131 merge_ready defect

Bug: buf->datalen >= 0x7fffffff

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version: Tor: 0.4.2.5
Severity: Minor Keywords:
Cc: 043-backport Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

With

BandwidthRate

set greater than 2147483646 bytes, for example Config line:

BandwidthRate 2147483647
#same as
BandwidthRate 2 GBytes

no streams complete in my relay and this Bug message appears:

Feb 02 06:32:37.000 [warn] {BUG} tor_bug_occurred_(): Bug: buffers_tls.c:73: buf_read_from_tls: Non-fatal assertion !(ASSERT_PREDICT
_UNLIKELY_(buf->datalen >= 0x7fffffff - at_most)) failed. (Future instances of this warning will be silenced.) (on Tor 0.4.2.5 )
Feb 02 06:32:37.000 [warn] {BUG} Bug: Tor 0.4.2.5: Non-fatal assertion !(ASSERT_PREDICT_UNLIKELY_(buf->datalen >= 0x7fffffff - at_mo
st)) failed in buf_read_from_tls at buffers_tls.c:73. (Stack trace not available) (on Tor 0.4.2.5 )

Looks like some INT_MAX buffer count trouble to me at least.

# BandwidthRate BandwidthRate __N__ bytes|KBytes|MBytes|GBytes|TBytes|KBits|MBits|GBits|TBits
#     A token bucket limits the average incoming bandwidth usage on this node
#     to the specified number of bytes per second, and the average outgoing
#     bandwidth usage to that same value.  If you want to run a relay in the
#     public network, this needs to be _at the very least_ 75 KBytes for a
#     relay (that is, 600 kbits) or 50 KBytes for a bridge (400 kbits) -- but of
#     course, more is better; we recommend at least 250 KBytes (2 mbits) if
#     possible.  (Default: 1 GByte) +
#  +
#     Note that this option, and other bandwidth-limiting options, apply to TCP
#     data only: They do not count TCP headers or DNS traffic. +
#  +
#     With this option, and in other options that take arguments in bytes,
#     KBytes, and so on, other formats are also supported. Notably, "KBytes" can
#     also be written as "kilobytes" or "kb"; "MBytes" can be written as
#     "megabytes" or "MB"; "kbits" can be written as "kilobits"; and so forth.
#     Tor also accepts "byte" and "bit" in the singular.
#     The prefixes "tera" and "T" are also recognized.
#     If no units are given, we default to bytes.
#     To avoid confusion, we recommend writing "bytes" or "bits" explicitly,
#     since it's easy to forget that "B" means bytes, not bits.

Child Tickets

Change History (16)

comment:1 Changed 9 months ago by nickm

Milestone: Tor: unspecified

comment:2 Changed 9 months ago by nickm

Is this on a 32-bit or 64-bit system? And do you really have 2 gigabytes per second of bandwidth?

comment:3 in reply to:  2 Changed 9 months ago by cypherpunks

Replying to nickm:

Is this on a 32-bit or 64-bit system?

X64 mingw x64

do you really have 2 gigabytes per second of bandwidth?

sadly no. not even 10GE.

max value for this is INT_MAX but it gives the bug message. if set 1 single byte below the MAX, it works without problem. results into:

BandwidthRate may should be at most 2147483646

getconf BandwidthRate
250 BandwidthRate=1073741824
setconf BandwidthRate="2 GBytes"
250 OK
getconf BandwidthRate
250 BandwidthRate=2147483647
setconf BandwidthRate="4 GBytes"
513 Unacceptable option value: BandwidthRate (4294967295) must be at most 2147483647

comment:4 Changed 9 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.4.x-final
Priority: MediumLow

Okay. We should fix this as correctness issue, but from what I can tell the solution for now is "don't set bandwidthrate so high" -- so we can leave it alone in 0.4.3.

comment:5 Changed 8 months ago by cypherpunks

The at_most value might be way too high. Initial patch here:

https://gitgud.io/onionk/tor/compare/master...inbufoverflow1

comment:6 Changed 8 months ago by nickm

Priority: LowMedium
Status: newneeds_review

comment:7 Changed 8 months ago by dgoulet

Reviewer: nickm

comment:8 Changed 8 months ago by nickm

Status: needs_reviewneeds_revision

Hm. That does seem like a good start. If we're going to merge it, I'd suggest some changes:

  • We need a changes file.
  • Logging a warning here would get extremely loud -- we probably shouldn't be doing that without a rate-limiter.
  • If we do want to log size_t values, we should be using TOR_PRIuSZ, not just casting to long.
  • The value of CONN_INBUF_MAX would make more sense if it were based on some value exposed by buffers.h.
  • A test here would be helpful too.

More broadly, though: if this behavior is the logical consequence of setting a very high bandwidth, should we disallow setting the bandwidth that high, or issue a warning if people try to do so?

comment:9 in reply to:  8 ; Changed 7 months ago by cypherpunks

Redid the branch a bit. Not sure how to test it.

Replying to nickm:

Yeah, the logging shouldn't be in the final version. It would just help to confirm which number is the one that's extremely high: at_most, or buf->datalen.

Logging would definitely be too loud, considering #31036 and #32022.

(Historical note, this particular check was added in #21369.)

So the high burst value, which becomes at_most later, is set in connection_or_update_token_buckets_helper. Doesn't it comes from the BandwidthBurst option, not BandwidthRate?

As long as we add this sanity check, is there a problem with keeping this behavior of using that high value from the user's configuration? at_most serves as an upper bound on what can be read from the socket in one go, so it has to be limited by something like BUF_MAX_LEN, but the burst limit in the token bucket doesn't have to be limited by that. The burst limit persists across multiple calls to connection_handle_read() (and any draining of inbuf in between calls).

comment:10 Changed 7 months ago by nickm

Status: needs_revisionneeds_review

comment:11 in reply to:  9 Changed 7 months ago by nickm

Replying to cypherpunks:

Redid the branch a bit. Not sure how to test it.

Replying to nickm:

Yeah, the logging shouldn't be in the final version. It would just help to confirm which number is the one that's extremely high: at_most, or buf->datalen.

One possibility if you want to keep the logging is to use log_ratelim(), to prevent it from writing the message too often.

Logging would definitely be too loud, considering #31036 and #32022.

(Historical note, this particular check was added in #21369.)

So the high burst value, which becomes at_most later, is set in connection_or_update_token_buckets_helper. Doesn't it comes from the BandwidthBurst option, not BandwidthRate?

Ultimately, yes, though I think there are other things that influence it.

As long as we add this sanity check, is there a problem with keeping this behavior of using that high value from the user's configuration? at_most serves as an upper bound on what can be read from the socket in one go, so it has to be limited by something like BUF_MAX_LEN, but the burst limit in the token bucket doesn't have to be limited by that. The burst limit persists across multiple calls to connection_handle_read() (and any draining of inbuf in between calls).

Hm. I guess this might not be a problem, though I'd be surprised if the code as it stands actually could deliver such a high burst.

comment:12 Changed 7 months ago by nickm

I've made a test merge branch at https://github.com/torproject/tor/pull/1834 so that I can let the CI run at this. There were some conflicts I had to resolve.

I'm thinking that the BUF_MAX_LEN changes are pervasive enough that we wouldn't want to backport those, if we do a backport here.

Any chance of getting a unit test for the new code here?

comment:13 in reply to:  12 Changed 7 months ago by cypherpunks

Replying to nickm:

Any chance of getting a unit test for the new code here?

I have no idea how to begin to create a unit test that replicates the conditions of this bug when calling connection_handle_read(), there aren't any pre existing unit tests of that function to learn from example.

For better merging to 0.3.5:

https://gitgud.io/onionk/tor/compare/master...inbufoverflow1-035

For 0.4.3:

https://gitgud.io/onionk/tor/compare/master...inbufoverflow1-043

comment:14 Changed 6 months ago by nickm

Cc: 043-backport added
Status: needs_reviewmerge_ready

Looks good. I think we should take this in master. We should consider a backport, though I'm on the fence about that.

comment:15 Changed 6 months ago by nickm

(Just realized I should run CI on this before I merge. I made a github PR at https://github.com/torproject/tor/pull/1868)

comment:16 Changed 6 months ago by nickm

Milestone: Tor: 0.4.4.x-finalTor: 0.4.3.x-final

Okay. CI is passing and this still looks good. Merging to master and marking for possible backport.

Note: See TracTickets for help on using tickets.