The last hop in vanguard circuits can be an RP/IP/HSDir.
Since vanguard circuits are at least 3 hops (sometimes 4) before this node,
this change will not cause A - B - A paths.
When parsing a vote in routerparse.c, only dirauth extract the commits from
the vote so move all this code into dirvote.c so we can make it specific to
the dirauth module.
If the dirauth module is disabled, the commit parsing does nothing.
Signed-off-by: David Goulet <dgoulet@torproject.org>
From dirvote.c to networkstatus.c where it makes more sense both in terms of
namespace and subsystem responsability.
This removes one less dependency on the dirauth module.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Add static inline dirauth public functions used outside of the dirauth module
so they can be seen by the tor code but simply do nothing.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Move most of the shared random functions that are needed outside of the
dirauth module.
At this commit, because dirvote.c hasn't been refactor, it doesn't compile
because some SR functions need a dirvote function.
Furthermore, 5 functions haven't been touched yet because they are dirauth
only but are in used in other C files than the dirauth module ones.
No code behavior change. Only moving code around.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is a pretty big commit but it only moves these files to src/or/dirauth:
dircollate.c dirvote.c shared_random.c shared_random_state.c
dircollate.h dirvote.h shared_random.h shared_random_state.h
Then many files are modified to change the include line for those header files
that have moved into a new directory.
Without using --disable-module-dirauth, everything builds fine. When using the
flag to disable the module, tor doesn't build due to linking errors. This will
be addressed in the next commit(s).
No code behavior change.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Remove useless include.
Clearly identify functions that are used by other part of Tor, functions that
are only used by the dirauth subsystem and functions that are exposed for unit
tests.
This will help us in the dirauth modularization effort.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Many functions become static to the C file or exposed to the tests within the
PRIVATE define of dirvote.h.
This commit moves a function to the top. No code behavior change.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Don't access the AuthoritativeDir options directly. We do this so we can move
authdir_mode() to the dirauth module.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Make our build system support a disable dirauth module option. It can only be
disabled explicitly with:
$ ./configure --disable-module-dirauth
If *not* specified that is enabled, an automake conditional variable is set to
true and a defined value for the C code:
AM_CONDITIONAL: BUILD_MODULE_DIRAUTH
AC_DEFINE: HAVE_MODULE_DIRAUTH=1
This introduces the dirauth/ module directory in src/or/ for which .c files
are only compiled if the BUILD_MODULE_DIRAUTH is set.
All the header files are compiled in regardless of the support so we can use
the alternative entry point functions of the dirauth subsystem.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Because we rescan the main loop event list if the global map of services has
changed, this makes sure it does work.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Because ADD_ONION/DEL_ONION can modify the global service map (both for v2 and
v3), we need to rescan the event list so we either enable or disable the HS
service main loop event.
Fixees #25939
Signed-off-by: David Goulet <dgoulet@torproject.org>
This is done because it makes our life easier with unit tests. Also, a rescan
on an uninitialized event list will result in a stacktrace.
Signed-off-by: David Goulet <dgoulet@torproject.org>
When we change the hibernation state, rescan the main loop event list because
the new state might affect the events.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Implement the ability to set flags per events which influences the set up of
the event.
This commit only adds one flag which is "need network" meaning that the event
is not enabled if tor has disabled the network or if hibernation mode.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Prior to #23100, we were not counting HS circuit build times in our
calculation of the timeout. This could lead to a condition where our timeout
was set too low, based on non HS circuit build times, and then we would
abandon all HS circuits, storing no valid timeouts in the histogram.
This commit avoids the assert.
Needed to run tests from the tarball else the geoip unit test would fail by
not finding that file.
Signed-off-by: David Goulet <dgoulet@torproject.org>
In 25374, we created the necessary post-loop event for scheduling
connection_ap_attach_pending as needed. Before that, we were
already running this event once per mainloop. There's no reason to
also run it once per second.
Closes ticket 25933. No changes file, since the relevant change is
already in 25374. Or possibly in 17590, depending on how you look
at it.
Our main function, though accurate on all platforms, can be very
slow on 32-bit hosts. This one is faster on all 32-bit hosts, and
accurate everywhere except apple, where it will typically be off by
1%. But since 32-bit apple is a relic anyway, I think we should be
fine.
Previously were using this value to have a cheap highish-resolution
timer. But we were only using it in one place, and current dogma is
to use monotime_coarse_t for this kind of thing.
This part of the code was the only part that used "cached
getttimeofday" feature, which wasn't monotonic, which we updated at
slight expense, and which I'd rather not maintain.
The clean_consdiffmgr() callback is only for relays acting as a directory
server, not all relays.
This commit adds a role for only directory server and sets the
clean_consdiffmgr() callback to use it.
Signed-off-by: David Goulet <dgoulet@torproject.org>
We can't end up in the removed else {} condition since we first validate the
flavor we get and then we validate the flavor we parse from the given
consensus which means we can only handle the two flavors of the if/elseif
conditions.
Fixes#25914
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously, we were ignoring values _over_ EPSILON. This bug was
also causing a warning at startup because the default value is set
to -1.0.
Fixes bug 25577; bugfix on 6b1dba214d. Bug not in any released tor.
Two helper functions to enable an event and disable an event which wraps the
launch and destroy of an event but takes care of the enabled flag.
They are also idempotent that is can be called multiple time on the same event
without effect if the event was already enabled or disabled.
Signed-off-by: David Goulet <dgoulet@torproject.org>
In case we transitionned to a new role in Tor, we need to launch and/or
destroy some periodic events.
Signed-off-by: David Goulet <dgoulet@torproject.org>
In tor, we have a series of possible "roles" that the tor daemon can be
enabled for. They are:
Client, Bridge, Relay, Authority (directory or bridge) and Onion service.
They can be combined sometimes. For instance, a Directory Authority is also a
Relay. This adds a "roles" field to a periodic event item object which is used
to know for which roles the event is for.
The next step is to enable the event only if the roles apply. No behavior
change at this commit.
Pars of #25762
Signed-off-by: David Goulet <dgoulet@torproject.org>
No behavior change, just to make it easier to find callbacks and for the sake
of our human brain to parse the list properly.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Consensus method 25 is the oldest one supported by any stable
version of 0.2.9, which is our current most-recent LTS. Thus, by
proposal 290, they should be removed.
This commit does not actually remove the code to implement these
methods: it only makes it so authorities will no longer support
them. I'll remove the backend code for them in later commits.
It tried to pick nodes for which only routerinfo_t items are set,
but without setting UseMicroDescriptors to 0. This won't work any
more, now that we're strict about using the right descriptor types
due to 25691/25692/25213.
In order to fix 25691 and 25692, we need to pass the "direct_conn"
flag to more places -- particularly when choosing single-hop
tunnels. The right way to do this involves having a couple more
functions accept router_crn_flags_t, rather than a big list of
boolean arguments.
This commit also makes sure that choose_good_exit_server_general()
honors the direct_conn flag, to fix 25691 and 25692.
In router_add_running_nodes_to_smartlist(), we had an inline
implementation of the logic from node_has_descriptor(), which should
be changed to node_has_preferred_descriptor().
This patch adds a new node_has_preferred_descriptor() function, and
replaces most users of node_has_descriptor() with it. That's an
important change, since as of d1874b4339 (our fix for #25213),
we are willing to say that a node has _some_ descriptor, but not the
_right_ descriptor for a particular use case.
Part of a fix for 25691 and 25692.
Now that we allow cpuworkers for dirport-only hosts (to fix 23693),
we need to allow dup_onion_keys() to succeed for them.
The change to construct_ntor_key_map() is for correctness,
but is not strictly necessary.
This is done as follows:
* Only one function (find_dl_schedule()) actually returned a
smartlist. Now it returns an int.
* The CSV_INTERVAL type has been altered to ignore everything
after the first comma, and to store the value before the first
comma in an int.
This commit won't compile. It was made with the following perl
scripts:
s/smartlist_t \*(.*)DownloadSchedule;/int $1DownloadInitialDelay;/;
s/\b(\w*)DownloadSchedule\b/$1DownloadInitialDelay/;
sizeof(ret) is the size of the pointer, not the size of what it
points to. Fortunately, we already have a function to compare
tor_addr_port_t values for equality.
Bugfix on c2c5b13e5d8a77e; bug not in any released Tor. Found by
clang's scan-build.
Now that we update our buckets on demand before reading or writing,
we no longer need to update them all every TokenBucketRefillInterval
msec.
When a connection runs out of bandwidth, we do need a way to
reenable it, however. We do this by scheduling a timer to reenable
all blocked connections for TokenBucketRefillInterval msec after a
connection becomes blocked.
(If we were using PerConnBWRate more, it might make sense to have a
per-connection timer, rather than a single timeout. But since
PerConnBWRate is currently (mostly) unused, I'm going to go for the
simpler approach here, since usually whenever one connection has
become blocked on bandwidth, most connections are blocked on
bandwidth.)
Implements ticket 25373.
Previously this was done as part of the refill callback, but there's
no real reason to do it like that. Since we're trying to remove the
refill callback completely, we can do this work as part of
record_num_bytes_transferred_impl(), which already does quite a lot
of this.
We used to do this 10x per second in connection_buckets_refill();
instead, we now do it when the bucket becomes empty. This change is
part of the work of making connection_buckets_refill() obsolete.
Closes ticket 25828; bugfix on 0.2.3.5-alpha.
We recently merged a circuit cell queue size safeguard. This commit adds the
number of killed circuits that have reached the limit to the DoS heartbeat. It
now looks like this:
[notice] DoS mitigation since startup: 0 circuits killed with too many
cells. 0 circuits rejected, 0 marked addresses. 0 connections closed. 0
single hop clients refused.
Second thing that this patch does. It makes tor always print the DoS
mitigation heartbeat line (for a relay) even though no DoS mitigation have
been enabled. The reason is because we now kill circuits that have too many
cells regardless on if it is enabled or not but also it will give the operator
a chance to learn what is enabled with the heartbeat instead of suddenly
appearing when it is enabled by let say the consensus.
Fixes#25824
Signed-off-by: David Goulet <dgoulet@torproject.org>
Unfortunately, the units passed to
monotime_coarse_stamp_units_to_approx_msec() was always 0 due to a type
conversion.
Signed-off-by: David Goulet <dgoulet@torproject.org>
This commit introduces the consensus parameter "circ_max_cell_queue_size"
which controls the maximum number of cells a circuit queue should have.
The default value is currently 50000 cells which is above what should be
expected but keeps us a margin of error for padding cells.
Related to this is #9072. Back in 0.2.4.14-alpha, we've removed that limit due
to a Guard discovery attack. Ticket #25226 details why we are putting back the
limit due to the memory pressure issue on relays.
Fixes#25226
Signed-off-by: David Goulet <dgoulet@torproject.org>
Both header and code file had some indentation issues after mass renaming.
No code behavior change.
Signed-off-by: David Goulet <dgoulet@torproject.org>
Really, the uint32_t is only an optimization; any kind of unit
should work fine. Some users might want to use time_t or
monotime_coarse_t or something like that.
Begin by creating a lowest-level triple of the types needed to
implement a token bucket: a configuration, a timestamp, and the raw
bucket itself.
Note that for low-level buckets, the units of the timestamp and the
bucket itself are unspecified: each user can use a different type.
(This patch breaks check-spaces; a later patch will fix it)
This is a simple search-and-replace to rename the token bucket type
to indicate that it contains both a read and a write bucket, bundled
with their configuration. It's preliminary to refactoring the
bucket type.
This test works by having two post-loop events activate one another
in a tight loop. If the "post-loop" mechanism didn't work, this
would be enough to starve all other events.
A linked connection_t is one that gets its I/O, not from the
network, but from another connection_t. When such a connection has
something to write, we want the corresponding connection to run its
read callback ... but not immediately, to avoid infinite recursion
and/or event loop starvation.
Previously we handled this case by activating the read events
outside the event loop. Now we use the "postloop event" logic.
This lets us simplify do_main_loop_once() a little.
We've been labeling some events as happening "outside the event
loop", to avoid Libevent starvation. This patch provides a cleaner
mechanism to avoid that starvation.
For background, the problem here is that Libevent only scans for new
events once it has run all its active callbacks. So if the
callbacks keep activating new callbacks, they could potentially
starve Libevent indefinitely and keep it from ever checking for
timed, socket, or signal events.
To solve this, we add the ability to label some events as
"post-loop". The rule for a "post-loop" event is that any events
_it_ activates can only be run after libevent has re-scanned for new
events at least once.
This differs from our previous token bucket abstraction in a few
ways:
1) It is an abstraction, and not a collection of fields.
2) It is meant to be used with monotonic timestamps, which should
produce better results than calling gettimeofday over and over.
In d1874b4339, we adjusted this check so that we insist on
using routerinfos for bridges. That's almost correct... but if we
have a bridge that is also a regular relay, then we should use
insist on its routerinfo when connecting to it as a bridge
(directly), and be willing to use its microdescriptor when
connecting to it elsewhere in our circuits.
This bug is a likely cause of some (all?) of the (exit_ei == NULL)
failures we've been seeing.
Fixes bug 25691; bugfix on 0.3.3.4-alpha
When size_t is 32 bits, the unit tests can't fit anything more than
4GB-1 into a size_t.
Additionally, tt_int_op() uses "long" -- we need tt_u64_op() to
safely test uint64_t values for equality.
Bug caused by tests for #24782 fix; not in any released Tor.
This patch changes the algorithm of compute_real_max_mem_in_queues() to
use 0.4 * RAM iff the system has more than or equal to 8 GB of RAM, but
will continue to use the old value of 0.75 * RAM if the system have less
than * GB of RAM available.
This patch also adds tests for compute_real_max_mem_in_queues().
See: https://bugs.torproject.org/24782
This patch makes get_total_system_memory mockable, which allows us to
alter the return value of the function in tests.
See: https://bugs.torproject.org/24782
This one happens if for some reason you start with DirPort enabled
but server mode turned off entirely.
Fixes a case of bug 23693; bugfix on 0.3.1.1-alpha.
This roughly doubles our test coverage of the bridges.c module.
* ADD new testing module, .../src/test/test_bridges.c.
* CHANGE a few function declarations from `static` to `STATIC`.
* CHANGE one function in transports.c, transport_get_by_name(), to be
mockable.
* CLOSES#25425: https://bugs.torproject.org/25425
This patch lifts the list of default directory authorities from config.c
into their own auth_dirs.inc file, which is then included in config.c
using the C preprocessor.
Patch by beastr0.
See: https://bugs.torproject.org/24854
* ADD new /src/common/crypto_rand.[ch] module.
* ADD new /src/common/crypto_util.[ch] module (contains the memwipe()
function, since all crypto_* modules need this).
* FIXES part of #24658: https://bugs.torproject.org/24658
This module doesn't actually need to mock the libevent mainloop at
all: it can just use the regular mainloop that the test environment
sets up.
Part of ticket 23750.
This change makes cpuworker and test_workqueue no longer need to
include event2/event.h. Now workqueue.c needs to include it, but
that is at least somewhat logical here.
We are going to stop recommending 0.2.5 so there is no reason to keep the
undef statement anymore.
Fixes#20522.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Both in geoip_note_client_seen() and options_need_geoip_info(), switch from
accessing the options directly to using the should_record_bridge_info() helper
function.
Fixes#25290
Signed-off-by: David Goulet <dgoulet@torproject.org>
Next commit is addressing the circuit queue cell limit so cleanup before doing
anything else.
Part of #25226
Signed-off-by: David Goulet <dgoulet@torproject.org>
Previously, the limit for MAX_PROTOCOLS_TO_EXPAND was actually being applied
in Rust to the maximum number of version (total, for all subprotocols).
Whereas in C, it was being applied to the number of subprotocols that were
allowed. This changes the Rust to match C's behaviour.
The behaviours still do not match, unsurprisingly, but now we know where a
primary difference is: the Rust is validating version ranges more than the C,
so in the C it's possible to call protover_all_supported on a ridiculous
version range like "Sleen=0-4294967294" because the C uses
MAX_PROTOCOLS_TO_EXPAND to count the number of *subprotocols* whereas the Rust
uses it to count the total number of *versions* of all subprotocols.
There's now no difference in these tests w.r.t. the C or Rust: both
fail miserably (well, Rust fails with nice descriptive errors, and C
gives you a traceback, because, well, C).
The DoS potential is slightly higher in C now due to some differences to the
Rust code, see the C_RUST_DIFFERS tags in src/rust/protover/tests/protover.rs.
Also, the comment about "failing at the splitting stage" in Rust wasn't true,
since when we split, we ignore empty chunks (e.g. "1--1" parses into
"(1,None),(None,1)" and "None" can't be parsed into an integer).
Finally, the comment about "Rust seems to experience an internal error" is only
true in debug mode, where u32s are bounds-checked at runtime. In release mode,
code expressing the equivalent of this test will error with
`Err(ProtoverError::Unparseable)` because 4294967295 is too large.
Previously, if "Link=1-5" was supported, and you asked protover_all_supported()
(or protover::all_supported() in Rust) if it supported "Link=3-999", the C
version would return "Link=3-999" and the Rust would return "Link=6-999". These
both behave the same now, i.e. both return "Link=6-999".
During code review and discussion with Chelsea Komlo, she pointed out
that protover::compute_for_old_tor() was a public function whose
return type was `&'static CStr`. We both agree that C-like parts of
APIs should:
1. not be exposed publicly (to other Rust crates),
2. only be called in the appropriate FFI code,
3. not expose types which are meant for FFI code (e.g. `*mut char`,
`CString`, `*const c_int`, etc.) to the pure-Rust code of other
crates.
4. FFI code (e.g. things in `ffi.rs` modules) should _never_ be called
from pure-Rust, not even from other modules in its own crate
(i.e. do not call `protover::ffi::*` from anywhere in
`protover::protoset::*`, etc).
With that in mind, this commit makes the following changes:
* CHANGE `protover::compute_for_old_tor()` to be
visible only at the `pub(crate)` level.
* RENAME `protover::compute_for_old_tor()` to
`protover::compute_for_old_tor_cstr()` to reflect the last change.
* ADD a new `protover::compute_for_old_tor()` function wrapper which
is public and intended for other Rust code to use, which returns a
`&str`.
It was changed to take borrows instead of taking ownership.
* REFACTOR `protover::ffi::protover_is_supported_here()` to use changed method
signature on `protover::is_supported_here()`.
This includes a subtle difference in behaviour to the previous Rust
implementation, where, for each vote that we're computing over, if a single one
fails to parse, we skip it. This now matches the current behaviour in the C
implementation.
* REFACTOR `protover::ffi::protover_compute_vote()` to use
new types and methods.
This includes a subtle difference in behaviour, as in 4258f1e18, where we return
(matching the C impl's return behaviour) earlier than before if parsing failed,
saving us computation in parsing the versions into a
protover::protoset::ProtoSet.
* REFACTOR `protover::ffi::protover_list_supports_protocol_or_later()` to use
new types and methods.
This includes a subtle difference in behaviour, as in 4258f1e18, where we return
(matching the C impl's return behaviour) earlier than before if parsing failed,
saving us computation in parsing the versions into a
protover::protoset::ProtoSet.
* REFACTOR `protover::ffi::protover_list_supports_protocol()` to use new types
and methods.
This includes differences in behaviour to before, which should now more closely
match the C version:
- If parsing a protover `char*` from C, and the string is not parseable, this
function will return 1 early, which matches the C behaviour when protocols
are unparseable. Previously, we would parse it and its version numbers
simultaneously, i.e. there was no fail early option, causing us to spend more
time unnecessarily parsing versions.
* REFACTOR `protover::ffi::protover_all_supported()` to use new types and
methods.
Previously, the rust implementation of protover considered an empty string to be
a valid ProtoEntry, while the C version did not (it must have a "=" character).
Other differences include that unknown protocols must now be parsed as
`protover::UnknownProtocol`s, and hence their entries as
`protover::UnvalidatedProtoEntry`s, whereas before (nearly) all protoentries
could be parsed regardless of how erroneous they might be considered by the C
version.
My apologies for this somewhat messy and difficult to read commit, if any part
is frustrating to the reviewer, please feel free to ask me to split this into
smaller changes (possibly hard to do, since so much changed), or ask me to
comment on a specific line/change and clarify how/when the behaviours differ.
The tests here should more closely match the behaviours exhibited by the C
implementation, but I do not yet personally guarantee they match precisely.
* REFACTOR unittests in protover::protover.
* ADD new integration tests for previously untested behaviour.
* FIXES part of #24031: https://bugs.torproject.org/24031.
This changes `protover::is_supported_here()` to be aware of new datatypes
(e.g. don't call `.0` on things which are no longer tuple structs) and also
changes the method signature to take borrows, making it faster, threadable, and
easier to read (i.e. the caller can know from reading the function signature
that the function won't mutate values passed into it).
* CHANGE the `protover::is_supported_here()` function to take borrows.
* REFACTOR the `protover::is_supported_here()` function to be aware of new
datatypes.
* FIXES part of #24031: https://bugs.torproject.org/24031
This adds a new type for votes upon `protover::ProtoEntry`s (technically, on
`protover::UnvalidatedProtoEntry`s, because the C code does not validate based
upon currently known protocols when voting, in order to maintain
future-compatibility), and converts several functions which would have operated
on this datatype into methods for ease-of-use and readability.
This also fixes a behavioural differentce to the C version of
protover_compute_vote(). The C version of protover_compute_vote() calls
expand_protocol_list() which checks if there would be too many subprotocols *or*
expanded individual version numbers, i.e. more than MAX_PROTOCOLS_TO_EXPAND, and
does this *per vote* (but only in compute_vote(), everywhere else in the C seems
to only care about the number of subprotocols, not the number of individual
versions). We need to match its behaviour in Rust and ensure we're not allowing
more than it would to get the votes to match.
* ADD new `protover::ProtoverVote` datatype.
* REMOVE the `protover::compute_vote()` function and refactor it into an
equivalent-in-behaviour albeit more memory-efficient voting algorithm based
on the new underlying `protover::protoset::ProtoSet` datatype, as
`ProtoverVote::compute()`.
* REMOVE the `protover::write_vote_to_string()` function, since this
functionality is now generated by the impl_to_string_for_proto_entry!() macro
for both `ProtoEntry` and `UnvalidatedProtoEntry` (the latter of which is the
correct type to return from a voting protocol instance, since the entity
voting may not know of all protocols being voted upon or known about by other
voting parties).
* FIXES part of #24031: https://bugs.torproject.org/24031
rust: Fix a difference in compute_vote() behaviour to C version.
This implements conversions from either a ProtoEntry or an UnvalidatedProtoEntry
into a String, for use in replacing such functions as
`protover::write_vote_to_string()`.
* ADD macro for implementing ToString trait for ProtoEntry and
UnvalidatedProtoEntry.
* FIXES part of #24031: https://bugs.torproject.org/24031
This adds a new protover::UnvalidatedProtoEntry type, which is the
UnknownProtocol variant of a ProtoEntry, and refactors several functions which
should operate on this type into methods.
This also fixes what was previously another difference to the C implementation:
if you asked the C version of protovet_compute_vote() to compute a single vote
containing "Fribble=", it would return NULL. However, the Rust version would
return "Fribble=" since it didn't check if the versions were empty before
constructing the string of differences. ("Fribble=" is technically a valid
protover string.) This is now fixed, and the Rust version in that case will,
analogous to (although safer than) C returning a NULL, return None.
* REMOVE internal `contains_only_supported_protocols()` function.
* REMOVE `all_supported()` function and refactor it into
`UnvalidatedProtoEntry::all_supported()`.
* REMOVE `parse_protocols_from_string_with_no_validation()` and
refactor it into the more rusty implementation of
`impl FromStr for UnvalidatedProtoEntry`.
* REMOVE `protover_string_supports_protocol()` and refactor it into
`UnvalidatedProtoEntry::supports_protocol()`.
* REMOVE `protover_string_supports_protocol_or_later()` and refactor
it into `UnvalidatedProtoEntry::supports_protocol_or_later()`.
* FIXES part of #24031: https://bugs.torproject.org/24031
rust: Fix another C/Rust different in compute_vote().
This fixes the unittest from the prior commit by checking if the versions are
empty before adding a protocol to a vote.
This replaces the `protover::SupportedProtocols` (why would you have a type just
for things which are supported?) with a new, more generic type,
`protover::ProtoEntry`, which utilises the new, more memory-efficient datatype
in protover::protoset.
* REMOVE `get_supported_protocols()` and `SupportedProtocols::tor_supported()`
(since they were never used separately) and collapse their functionality into
a single `ProtoEntry::supported()` method.
* REMOVE `SupportedProtocols::from_proto_entries()` and reimplement its
functionality as the more rusty `impl FromStr for ProtoEntry`.
* REMOVE `get_proto_and_vers()` function and refactor it into the more rusty
`impl FromStr for ProtoEntry`.
* FIXES part of #24031: https://bugs.torproject.org/24031
* ADD new type, protover::UnknownProtocol, so that we have greater type safety
and our protover functionality which works with unsanitised protocol names is
more clearly demarcated.
* REFACTOR protover::Proto, renaming it protover::Protocol to mirror the new
protover::UnknownProtocol type name.
* ADD a utility conversion of `impl From<Protocol> for UnknownProtocol` so that
we can easily with known protocols and unknown protocols simultaneously
(e.g. doing comparisons, checking their version numbers), while not allowing
UnknownProtocols to be accidentally used in functions which should only take
Protocols.
* FIXES part of #24031: https://bugs.torproject.org/24031
* ADD new protover::protoset module.
* ADD new protover::protoset::ProtoSet class for holding protover versions.
* REMOVE protover::Versions type implementation and its method
`from_version_string()`, and instead implement this behaviour in a more
rust-like manner as `impl FromStr for ProtoSet`.
* MOVE the `find_range()` utility function from protover::protover to
protover::protoset since it's only used internally in the
implementation of ProtoSet.
* REMOVE the `contract_protocol_list()` function from protover::protover and
instead refactor it (reusing nearly the entire thing, with minor superficial,
i.e. non-behavioural, changes) into a more rusty
`impl ToString for ProtoSet`.
* REMOVE the `expand_version_range()` function from protover::protover and
instead refactor it into a more rusty implementation of
`impl Into<Vec<Version>> for ProtoSet` using the new error types in
protover::errors.
* FIXES part of #24031: https://bugs.torproject.org/24031.
This will allow us to do actual error handling intra-crate in a more
rusty manner, e.g. propogating errors in match statements, conversion
between error types, logging messages, etc.
* FIXES part of #24031: https://bugs.torproject.org/24031.
Previously, the limit for MAX_PROTOCOLS_TO_EXPAND was actually being applied
in Rust to the maximum number of version (total, for all subprotocols).
Whereas in C, it was being applied to the number of subprotocols that were
allowed. This changes the Rust to match C's behaviour.
The behaviours still do not match, unsurprisingly, but now we know where a
primary difference is: the Rust is validating version ranges more than the C,
so in the C it's possible to call protover_all_supported on a ridiculous
version range like "Sleen=0-4294967294" because the C uses
MAX_PROTOCOLS_TO_EXPAND to count the number of *subprotocols* whereas the Rust
uses it to count the total number of *versions* of all subprotocols.
There's now no difference in these tests w.r.t. the C or Rust: both
fail miserably (well, Rust fails with nice descriptive errors, and C
gives you a traceback, because, well, C).
The DoS potential is slightly higher in C now due to some differences to the
Rust code, see the C_RUST_DIFFERS tags in src/rust/protover/tests/protover.rs.
Also, the comment about "failing at the splitting stage" in Rust wasn't true,
since when we split, we ignore empty chunks (e.g. "1--1" parses into
"(1,None),(None,1)" and "None" can't be parsed into an integer).
Finally, the comment about "Rust seems to experience an internal error" is only
true in debug mode, where u32s are bounds-checked at runtime. In release mode,
code expressing the equivalent of this test will error with
`Err(ProtoverError::Unparseable)` because 4294967295 is too large.
Previously, if "Link=1-5" was supported, and you asked protover_all_supported()
(or protover::all_supported() in Rust) if it supported "Link=3-999", the C
version would return "Link=3-999" and the Rust would return "Link=6-999". These
both behave the same now, i.e. both return "Link=6-999".
During code review and discussion with Chelsea Komlo, she pointed out
that protover::compute_for_old_tor() was a public function whose
return type was `&'static CStr`. We both agree that C-like parts of
APIs should:
1. not be exposed publicly (to other Rust crates),
2. only be called in the appropriate FFI code,
3. not expose types which are meant for FFI code (e.g. `*mut char`,
`CString`, `*const c_int`, etc.) to the pure-Rust code of other
crates.
4. FFI code (e.g. things in `ffi.rs` modules) should _never_ be called
from pure-Rust, not even from other modules in its own crate
(i.e. do not call `protover::ffi::*` from anywhere in
`protover::protoset::*`, etc).
With that in mind, this commit makes the following changes:
* CHANGE `protover::compute_for_old_tor()` to be
visible only at the `pub(crate)` level.
* RENAME `protover::compute_for_old_tor()` to
`protover::compute_for_old_tor_cstr()` to reflect the last change.
* ADD a new `protover::compute_for_old_tor()` function wrapper which
is public and intended for other Rust code to use, which returns a
`&str`.
It was changed to take borrows instead of taking ownership.
* REFACTOR `protover::ffi::protover_is_supported_here()` to use changed method
signature on `protover::is_supported_here()`.
This includes a subtle difference in behaviour to the previous Rust
implementation, where, for each vote that we're computing over, if a single one
fails to parse, we skip it. This now matches the current behaviour in the C
implementation.
* REFACTOR `protover::ffi::protover_compute_vote()` to use
new types and methods.
This includes a subtle difference in behaviour, as in 4258f1e18, where we return
(matching the C impl's return behaviour) earlier than before if parsing failed,
saving us computation in parsing the versions into a
protover::protoset::ProtoSet.
* REFACTOR `protover::ffi::protover_list_supports_protocol_or_later()` to use
new types and methods.
This includes a subtle difference in behaviour, as in 4258f1e18, where we return
(matching the C impl's return behaviour) earlier than before if parsing failed,
saving us computation in parsing the versions into a
protover::protoset::ProtoSet.
* REFACTOR `protover::ffi::protover_list_supports_protocol()` to use new types
and methods.
This includes differences in behaviour to before, which should now more closely
match the C version:
- If parsing a protover `char*` from C, and the string is not parseable, this
function will return 1 early, which matches the C behaviour when protocols
are unparseable. Previously, we would parse it and its version numbers
simultaneously, i.e. there was no fail early option, causing us to spend more
time unnecessarily parsing versions.
* REFACTOR `protover::ffi::protover_all_supported()` to use new types and
methods.
Previously, the rust implementation of protover considered an empty string to be
a valid ProtoEntry, while the C version did not (it must have a "=" character).
Other differences include that unknown protocols must now be parsed as
`protover::UnknownProtocol`s, and hence their entries as
`protover::UnvalidatedProtoEntry`s, whereas before (nearly) all protoentries
could be parsed regardless of how erroneous they might be considered by the C
version.
My apologies for this somewhat messy and difficult to read commit, if any part
is frustrating to the reviewer, please feel free to ask me to split this into
smaller changes (possibly hard to do, since so much changed), or ask me to
comment on a specific line/change and clarify how/when the behaviours differ.
The tests here should more closely match the behaviours exhibited by the C
implementation, but I do not yet personally guarantee they match precisely.
* REFACTOR unittests in protover::protover.
* ADD new integration tests for previously untested behaviour.
* FIXES part of #24031: https://bugs.torproject.org/24031.
This changes `protover::is_supported_here()` to be aware of new datatypes
(e.g. don't call `.0` on things which are no longer tuple structs) and also
changes the method signature to take borrows, making it faster, threadable, and
easier to read (i.e. the caller can know from reading the function signature
that the function won't mutate values passed into it).
* CHANGE the `protover::is_supported_here()` function to take borrows.
* REFACTOR the `protover::is_supported_here()` function to be aware of new
datatypes.
* FIXES part of #24031: https://bugs.torproject.org/24031
This adds a new type for votes upon `protover::ProtoEntry`s (technically, on
`protover::UnvalidatedProtoEntry`s, because the C code does not validate based
upon currently known protocols when voting, in order to maintain
future-compatibility), and converts several functions which would have operated
on this datatype into methods for ease-of-use and readability.
This also fixes a behavioural differentce to the C version of
protover_compute_vote(). The C version of protover_compute_vote() calls
expand_protocol_list() which checks if there would be too many subprotocols *or*
expanded individual version numbers, i.e. more than MAX_PROTOCOLS_TO_EXPAND, and
does this *per vote* (but only in compute_vote(), everywhere else in the C seems
to only care about the number of subprotocols, not the number of individual
versions). We need to match its behaviour in Rust and ensure we're not allowing
more than it would to get the votes to match.
* ADD new `protover::ProtoverVote` datatype.
* REMOVE the `protover::compute_vote()` function and refactor it into an
equivalent-in-behaviour albeit more memory-efficient voting algorithm based
on the new underlying `protover::protoset::ProtoSet` datatype, as
`ProtoverVote::compute()`.
* REMOVE the `protover::write_vote_to_string()` function, since this
functionality is now generated by the impl_to_string_for_proto_entry!() macro
for both `ProtoEntry` and `UnvalidatedProtoEntry` (the latter of which is the
correct type to return from a voting protocol instance, since the entity
voting may not know of all protocols being voted upon or known about by other
voting parties).
* FIXES part of #24031: https://bugs.torproject.org/24031
rust: Fix a difference in compute_vote() behaviour to C version.
This implements conversions from either a ProtoEntry or an UnvalidatedProtoEntry
into a String, for use in replacing such functions as
`protover::write_vote_to_string()`.
* ADD macro for implementing ToString trait for ProtoEntry and
UnvalidatedProtoEntry.
* FIXES part of #24031: https://bugs.torproject.org/24031
This adds a new protover::UnvalidatedProtoEntry type, which is the
UnknownProtocol variant of a ProtoEntry, and refactors several functions which
should operate on this type into methods.
This also fixes what was previously another difference to the C implementation:
if you asked the C version of protovet_compute_vote() to compute a single vote
containing "Fribble=", it would return NULL. However, the Rust version would
return "Fribble=" since it didn't check if the versions were empty before
constructing the string of differences. ("Fribble=" is technically a valid
protover string.) This is now fixed, and the Rust version in that case will,
analogous to (although safer than) C returning a NULL, return None.
* REMOVE internal `contains_only_supported_protocols()` function.
* REMOVE `all_supported()` function and refactor it into
`UnvalidatedProtoEntry::all_supported()`.
* REMOVE `parse_protocols_from_string_with_no_validation()` and
refactor it into the more rusty implementation of
`impl FromStr for UnvalidatedProtoEntry`.
* REMOVE `protover_string_supports_protocol()` and refactor it into
`UnvalidatedProtoEntry::supports_protocol()`.
* REMOVE `protover_string_supports_protocol_or_later()` and refactor
it into `UnvalidatedProtoEntry::supports_protocol_or_later()`.
* FIXES part of #24031: https://bugs.torproject.org/24031
rust: Fix another C/Rust different in compute_vote().
This fixes the unittest from the prior commit by checking if the versions are
empty before adding a protocol to a vote.
This replaces the `protover::SupportedProtocols` (why would you have a type just
for things which are supported?) with a new, more generic type,
`protover::ProtoEntry`, which utilises the new, more memory-efficient datatype
in protover::protoset.
* REMOVE `get_supported_protocols()` and `SupportedProtocols::tor_supported()`
(since they were never used separately) and collapse their functionality into
a single `ProtoEntry::supported()` method.
* REMOVE `SupportedProtocols::from_proto_entries()` and reimplement its
functionality as the more rusty `impl FromStr for ProtoEntry`.
* REMOVE `get_proto_and_vers()` function and refactor it into the more rusty
`impl FromStr for ProtoEntry`.
* FIXES part of #24031: https://bugs.torproject.org/24031
* ADD new type, protover::UnknownProtocol, so that we have greater type safety
and our protover functionality which works with unsanitised protocol names is
more clearly demarcated.
* REFACTOR protover::Proto, renaming it protover::Protocol to mirror the new
protover::UnknownProtocol type name.
* ADD a utility conversion of `impl From<Protocol> for UnknownProtocol` so that
we can easily with known protocols and unknown protocols simultaneously
(e.g. doing comparisons, checking their version numbers), while not allowing
UnknownProtocols to be accidentally used in functions which should only take
Protocols.
* FIXES part of #24031: https://bugs.torproject.org/24031
* ADD new protover::protoset module.
* ADD new protover::protoset::ProtoSet class for holding protover versions.
* REMOVE protover::Versions type implementation and its method
`from_version_string()`, and instead implement this behaviour in a more
rust-like manner as `impl FromStr for ProtoSet`.
* MOVE the `find_range()` utility function from protover::protover to
protover::protoset since it's only used internally in the
implementation of ProtoSet.
* REMOVE the `contract_protocol_list()` function from protover::protover and
instead refactor it (reusing nearly the entire thing, with minor superficial,
i.e. non-behavioural, changes) into a more rusty
`impl ToString for ProtoSet`.
* REMOVE the `expand_version_range()` function from protover::protover and
instead refactor it into a more rusty implementation of
`impl Into<Vec<Version>> for ProtoSet` using the new error types in
protover::errors.
* FIXES part of #24031: https://bugs.torproject.org/24031.
This will allow us to do actual error handling intra-crate in a more
rusty manner, e.g. propogating errors in match statements, conversion
between error types, logging messages, etc.
* FIXES part of #24031: https://bugs.torproject.org/24031.
When a relay is collecting internal statistics about how many
create cell requests it has seen of each type, accurately count the
requests from relays that temporarily fall out of the consensus.
(To be extra conservative, we were already ignoring requests from clients
in our counts, and we continue ignoring them here.)
Fixes bug 24910; bugfix on 0.2.4.17-rc.
Directory authorities no longer vote in favor of the Guard flag
for relays that don't advertise directory support.
Starting in Tor 0.3.0.1-alpha, Tor clients have been avoiding using
such relays in the Guard position, leading to increasingly broken load
balancing for the 5%-or-so of Guards that don't advertise directory
support.
Fixes bug 22310; bugfix on 0.3.0.6.
Add a missing lock acquisition around access to queued_control_events
in control_free_all(). Use the reassign-and-unlock strategy as in
queued_events_flush_all(). Fixes bug 25675. Coverity found this bug,
but only after we recently added an access to
flush_queued_event_pending.
Nick Mathewson
Marcin Cieślak
Neel Chauhan
Mike Perry
George Kadianakis
David Goulet
juga0
Isis Lovecruft
Deepesh Pathak
Alexander Færøy
Roger Dingledine
Fernando Fernandez Mancera
Karsten Loesing
Dhalgren
Taylor Yu