Trac issueshttps://gitlab.torproject.org/legacy/trac/-/issues2020-06-13T15:50:12Zhttps://gitlab.torproject.org/legacy/trac/-/issues/33039Memory leaks in handle_control_getconf2020-06-13T15:50:12ZNick MathewsonMemory leaks in handle_control_getconfFound while investigating #33006:
```
=================================================================
==180005==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x7fb54c5cdc5...Found while investigating #33006:
```
=================================================================
==180005==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 32 byte(s) in 1 object(s) allocated from:
#0 0x7fb54c5cdc58 in __interceptor_malloc (/lib64/libasan.so.5+0x10dc58)
#1 0x55a4af274f3a in tor_malloc_ src/lib/malloc/malloc.c:45
#2 0x55a4af274fd0 in tor_malloc_zero_ src/lib/malloc/malloc.c:71
#3 0x55a4af2371ca in config_line_append src/lib/encoding/confline.c:40
#4 0x55a4aeffaec7 in control_reply_add_one_kv src/feature/control/control_proto.c:345
#5 0x55a4aefdd2ae in handle_control_getconf src/feature/control/control_cmd.c:307
#6 0x55a4aefe54b2 in handle_single_control_command src/feature/control/control_cmd.c:2376
#7 0x55a4aefe54b2 in handle_control_command src/feature/control/control_cmd.c:2407
#8 0x55a4aefd6e01 in connection_control_process_inbuf src/feature/control/control.c:508
#9 0x55a4aeee0e01 in connection_handle_read_impl src/core/mainloop/connection.c:3737
#10 0x55a4aeee0e01 in connection_handle_read src/core/mainloop/connection.c:3777
#11 0x55a4aeeecf00 in conn_read_callback src/core/mainloop/mainloop.c:892
#12 0x7fb54c32abb6 (/lib64/libevent-2.1.so.6+0x25bb6)
Indirect leak of 26 byte(s) in 1 object(s) allocated from:
#0 0x7fb54c55652d in strdup (/lib64/libasan.so.5+0x9652d)
#1 0x55a4af2751d0 in tor_strdup_ src/lib/malloc/malloc.c:165
#2 0x55a4af2371d5 in config_line_append src/lib/encoding/confline.c:41
#3 0x55a4aeffaec7 in control_reply_add_one_kv src/feature/control/control_proto.c:345
#4 0x55a4aefdd2ae in handle_control_getconf src/feature/control/control_cmd.c:307
#5 0x55a4aefe54b2 in handle_single_control_command src/feature/control/control_cmd.c:2376
#6 0x55a4aefe54b2 in handle_control_command src/feature/control/control_cmd.c:2407
#7 0x55a4aefd6e01 in connection_control_process_inbuf src/feature/control/control.c:508
#8 0x55a4aeee0e01 in connection_handle_read_impl src/core/mainloop/connection.c:3737
#9 0x55a4aeee0e01 in connection_handle_read src/core/mainloop/connection.c:3777
#10 0x55a4aeeecf00 in conn_read_callback src/core/mainloop/mainloop.c:892
#11 0x7fb54c32abb6 (/lib64/libevent-2.1.so.6+0x25bb6)
Indirect leak of 7 byte(s) in 1 object(s) allocated from:
#0 0x7fb54c55652d in strdup (/lib64/libasan.so.5+0x9652d)
#1 0x55a4af2751d0 in tor_strdup_ src/lib/malloc/malloc.c:165
#2 0x55a4af2371f5 in config_line_append src/lib/encoding/confline.c:42
#3 0x55a4aeffaec7 in control_reply_add_one_kv src/feature/control/control_proto.c:345
#4 0x55a4aefdd2ae in handle_control_getconf src/feature/control/control_cmd.c:307
#5 0x55a4aefe54b2 in handle_single_control_command src/feature/control/control_cmd.c:2376
#6 0x55a4aefe54b2 in handle_control_command src/feature/control/control_cmd.c:2407
#7 0x55a4aefd6e01 in connection_control_process_inbuf src/feature/control/control.c:508
#8 0x55a4aeee0e01 in connection_handle_read_impl src/core/mainloop/connection.c:3737
#9 0x55a4aeee0e01 in connection_handle_read src/core/mainloop/connection.c:3777
#10 0x55a4aeeecf00 in conn_read_callback src/core/mainloop/mainloop.c:892
#11 0x7fb54c32abb6 (/lib64/libevent-2.1.so.6+0x25bb6)
SUMMARY: AddressSanitizer: 65 byte(s) leaked in 3 allocation(s).
Exit code was 1
success (15.21s)
```Tor: 0.4.3.x-finalTaylor YuTaylor Yuhttps://gitlab.torproject.org/legacy/trac/-/issues/25908make test suite coverage more deterministic2020-06-13T15:38:03ZTaylor Yumake test suite coverage more deterministicOne thing that appears to be happening in the coveralls integration is some small blocks of code seem to be changing coverage without obvious corresponding changes in source code or tests. This can cause false positive indications of de...One thing that appears to be happening in the coveralls integration is some small blocks of code seem to be changing coverage without obvious corresponding changes in source code or tests. This can cause false positive indications of decreased coverage reported in things like pull requests. We should eliminate as much of this indeterminacy as possible.
One example is https://coveralls.io/builds/16676967/source?filename=src%2For%2Fcircuitstats.c#L879
This ticket should probably have child tickets for specific instances of this problem.Tor: 0.3.4.x-finalhttps://gitlab.torproject.org/legacy/trac/-/issues/26014Fix two cases of nondeterminism in voting_schedule.c coverage2020-06-13T15:25:24ZNick MathewsonFix two cases of nondeterminism in voting_schedule.c coverageAfter another set of coverage-comparison testing, I found the following to cases of nondeterminism in the tests for voting_schedule.c:
```
--- a/voting_schedule.c.gcov
+++ b/voting_schedule.c.gcov
@@ -61,7 +61,7 @@
-:
...After another set of coverage-comparison testing, I found the following to cases of nondeterminism in the tests for voting_schedule.c:
```
--- a/voting_schedule.c.gcov
+++ b/voting_schedule.c.gcov
@@ -61,7 +61,7 @@
-:
1: next += offset;
1: if (next - interval > now)
- #####: next -= interval;
+ 1: next -= interval;
-:
1: return next;
-:}
```
```
--- a/voting_schedule.c.gcov
+++ b/voting_schedule.c.gcov
@@ -52,7 +52,7 @@
-:
-: /* Intervals never cross midnight. */
1: if (next > midnight_tomorrow)
- #####: next = midnight_tomorrow;
+ 1: next = midnight_tomorrow;
-:
-: /* If the interval would only last half as long as it's supposed to, then
-: * skip over to the next day. */
```
I think that these changes are probably dependent on using clock time for our tests, since they all happened around 0:00 UTC.Tor: 0.3.4.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/26008Make workqueue cancel code always get covered by tests.2020-06-13T15:25:23ZNick MathewsonMake workqueue cancel code always get covered by tests.After re-running the tests many times with the other #25908 children fixed, I found a remaining variance from the most usual test coverage. This case happened only once out of ~450 runs:
```
--- coverage-1525271181/workqueue.c.gcov.tmp ...After re-running the tests many times with the other #25908 children fixed, I found a remaining variance from the most usual test coverage. This case happened only once out of ~450 runs:
```
--- coverage-1525271181/workqueue.c.gcov.tmp 2018-05-02 15:59:24.758433136 -0400
+++ coverage-1525271827/workqueue.c.gcov.tmp 2018-05-02 15:59:24.760433142 -0400
@@ -198,14 +198,14 @@
1: tor_mutex_acquire(&ent->on_pool->lock);
1: workqueue_priority_t prio = ent->priority;
1: if (ent->pending) {
- 1: TOR_TAILQ_REMOVE(&ent->on_pool->work[prio], ent, next_work);
- 1: cancelled = 1;
- 1: result = ent->arg;
+ #####: TOR_TAILQ_REMOVE(&ent->on_pool->work[prio], ent, next_work);
+ #####: cancelled = 1;
+ #####: result = ent->arg;
-: }
1: tor_mutex_release(&ent->on_pool->lock);
-:
1: if (cancelled) {
- 1: workqueue_entry_free(ent);
+ #####: workqueue_entry_free(ent);
-: }
1: return result;
-:}
```Tor: 0.3.4.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/25997Solve nondeterminism in testing of hs_get_responsible_hsdirs2020-06-13T15:25:18ZNick MathewsonSolve nondeterminism in testing of hs_get_responsible_hsdirsOnly sometimes do our tests call the case of hs_get_responsible_hsdirs where it "wraps around":
```
/* Getting the length of the list if no member is greater than the key we
* are looking for so start at the first element. */
...Only sometimes do our tests call the case of hs_get_responsible_hsdirs where it "wraps around":
```
/* Getting the length of the list if no member is greater than the key we
* are looking for so start at the first element. */
if (idx == smartlist_len(sorted_nodes)) {
start = idx = 0;
}
```
We should make this consistent.
I plan to do this, but dgoulet or asn should steal it if they'd rather. :)Tor: 0.3.4.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/25996Produce consistent coverage from test_client_pick_intro()2020-06-13T15:25:18ZNick MathewsonProduce consistent coverage from test_client_pick_intro()This test can produce different coverage, depending on whether the non-excluded introduction point is chosen first or not. To prevent this, we can just repeat the relevant portion of the test enough times.This test can produce different coverage, depending on whether the non-excluded introduction point is chosen first or not. To prevent this, we can just repeat the relevant portion of the test enough times.Tor: 0.3.4.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/25995Use a deterministic PRNG in test_circuit_timeout() for predictable coverage.2020-06-13T15:25:18ZNick MathewsonUse a deterministic PRNG in test_circuit_timeout() for predictable coverage.The test coverage from test_circuit_timeout() is nondeterministic, because the function deliberately creates random samples and passes them to the circuitstats module.
I propose that for this function, we replace the RNG with a mocked r...The test coverage from test_circuit_timeout() is nondeterministic, because the function deliberately creates random samples and passes them to the circuitstats module.
I propose that for this function, we replace the RNG with a mocked replacement.Tor: 0.3.4.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/25994test_channel: keep time constant when running channel/outbound_cell2020-06-13T15:25:17ZNick Mathewsontest_channel: keep time constant when running channel/outbound_cellWith fairly low probability, the outbound_cell test could get run with cached_gettimeofday on a different 10-second boundary than the current value of gettimeofday, resulting in a call to scale_active_circuits().
Now that we no longer u...With fairly low probability, the outbound_cell test could get run with cached_gettimeofday on a different 10-second boundary than the current value of gettimeofday, resulting in a call to scale_active_circuits().
Now that we no longer use cached_gettimeofday, the probability of calling scale_active_circuits is even lower here. If the test takes about 200 microseconds (as it does on my desktop), we should only expect it to straddle the 10-second boundary with P < 200 usec / 10s == 1 / 50000.
Still, it's a good idea to fix this kind of thing.
This change will _lower_ coverage from the maximum by making it so this test does not call scale_active_circuits. However, this test does not actually validate any of the behavior of scale_active_circuits, so this change is appropriate.Tor: 0.3.4.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/25993Improve deliberate test coverage for addressmap_get_virtual_address()2020-06-13T15:25:17ZNick MathewsonImprove deliberate test coverage for addressmap_get_virtual_address()This function is currently tested indirectly from test_entryconn.c, but this results in unreliable coverage for a couple of the features in this function.
Specifically, we don't test whether duplicate addresses are rejected, or whether ...This function is currently tested indirectly from test_entryconn.c, but this results in unreliable coverage for a couple of the features in this function.
Specifically, we don't test whether duplicate addresses are rejected, or whether IPv4 addresses ending with .0 or .255 are rejected.
This is one source of nondeterminism in our test coverage.Tor: 0.3.4.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/23741scripts/test/coverage tries to mv .gcov files to /2020-06-13T15:15:04ZTaylor Yuscripts/test/coverage tries to mv .gcov files to /When run with no arguments, the `scripts/test/coverage` script attempts to move the .gcov files to the root directory. This shouldn't happen. Hopefully most users won't run this script as a UID that has write access to the root directo...When run with no arguments, the `scripts/test/coverage` script attempts to move the .gcov files to the root directory. This shouldn't happen. Hopefully most users won't run this script as a UID that has write access to the root directory, but we should fix this anyway. Either exit with a usage error if there is no directory name argument, or change the test to `[ -d "$dst" ]`. (The existing test, `[ -n $dst ]` with no quoting around `$dst`, returns true if `$dst` is empty on the macOS and Linux systems I've tried it on. This surprising behavior is apparently required by POSIX.)Tor: 0.3.2.x-finalTaylor YuTaylor Yuhttps://gitlab.torproject.org/legacy/trac/-/issues/16792Have a way to mark lines as "unreachable by unit tests"2020-06-13T15:15:02ZNick MathewsonHave a way to mark lines as "unreachable by unit tests"It would be great to have a way to tell the test coverage code "this line won't be reached by tests, so don't worry about it." This would let us have a prayer of reaching 100%.It would be great to have a way to tell the test coverage code "this line won't be reached by tests, so don't worry about it." This would let us have a prayer of reaching 100%.Tor: 0.2.9.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/23739improve documentation on how we use gcov2020-06-13T15:15:01ZTaylor Yuimprove documentation on how we use gcovTicket #16792 (0.2.9.1-alpha) introduced some automation for excluding lines from filtered gcov output. We should document the prefixes it uses to mark excluded lines. There should also be information about how to read the gcov-diff fi...Ticket #16792 (0.2.9.1-alpha) introduced some automation for excluding lines from filtered gcov output. We should document the prefixes it uses to mark excluded lines. There should also be information about how to read the gcov-diff files, which are processed to remove line numbers, among other things. We should also refer to the gcc documentation for gcov at https://gcc.gnu.org/onlinedocs/gcc/Invoking-Gcov.htmlTor: 0.3.2.x-finalTaylor YuTaylor Yuhttps://gitlab.torproject.org/legacy/trac/-/issues/16798Raise compat_* testing to over 80%2020-06-13T14:58:35ZNick MathewsonRaise compat_* testing to over 80%```
compat.c.gcov 360 426 54.20
compat_libevent.c.gcov 92 32 25.81
compat_pthreads.c.gcov 22 72 76.60
compat_threads.c.gcov 48 51 51.52
```
Getting 95% coverage for stuff that relies on OS functionality might not be feasible, but it wou...```
compat.c.gcov 360 426 54.20
compat_libevent.c.gcov 92 32 25.81
compat_pthreads.c.gcov 22 72 76.60
compat_threads.c.gcov 48 51 51.52
```
Getting 95% coverage for stuff that relies on OS functionality might not be feasible, but it would be great to get this as high as possible.
(Stubbing OS calls out is probably *not* the best answer here, even though you might think it would be. We're not only interested in whether our compatibility wrappers do the right thing when the OS behaves the way we expect: we're also interested in whether they do the right thing on the actual OS. )Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/16794All cryptography unit test coverage should be over 95%; all should have test ...2020-06-13T14:57:00ZNick MathewsonAll cryptography unit test coverage should be over 95%; all should have test vectorsIt's high, but it's not there yet:
```
x/crypto.c.gcov 204 738 78.34
x/crypto_curve25519.c.gcov 8 74 90.24
x/crypto_ed25519.c.gcov 17 95 84.82
x/crypto_format.c.gcov 5 95 95.00
x/crypto_pwbox.c.gcov 2 59 96.72
x/crypto_s2k.c.gcov 14 152...It's high, but it's not there yet:
```
x/crypto.c.gcov 204 738 78.34
x/crypto_curve25519.c.gcov 8 74 90.24
x/crypto_ed25519.c.gcov 17 95 84.82
x/crypto_format.c.gcov 5 95 95.00
x/crypto_pwbox.c.gcov 2 59 96.72
x/crypto_s2k.c.gcov 14 152 91.57
x/aes.c.gcov 0 17 100.00
TOTAL 250 1230 83.11
```Tor: 0.2.9.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/18668Numerous WSAStartup warnings in unit tests on windows2020-06-13T14:55:50ZNick MathewsonNumerous WSAStartup warnings in unit tests on windowsHead on over to Jenkins, and look at the windows builder output. It's complaining a lot that we aren't calling WSAStartup. We should fix that.
It's not causing bugs or preventing the tests from passing, but it sure is ugly.Head on over to Jenkins, and look at the windows builder output. It's complaining a lot that we aren't calling WSAStartup. We should fix that.
It's not causing bugs or preventing the tests from passing, but it sure is ugly.Tor: 0.2.9.x-finalNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/legacy/trac/-/issues/18665Make test_util_time pieces pass on windows2020-06-13T14:55:49ZNick MathewsonMake test_util_time pieces pass on windowsThere are some pieces in test_util_time that I disabled to make the tests pass. Presumably these never worked on Windows; but we should re-enable them and make them work.There are some pieces in test_util_time that I disabled to make the tests pass. Presumably these never worked on Windows; but we should re-enable them and make them work.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/17845Add unit tests for IPv6 relay descriptors2020-06-13T14:52:27ZteorAdd unit tests for IPv6 relay descriptorsOnce chutney can verify using relays on IPv6 (#17011/#17153), we should update the unit tests to use descriptors that contain IPv6 addresses in "or-address" lines.
This will allow us to unit test #17840 and various other tickets.
From ...Once chutney can verify using relays on IPv6 (#17011/#17153), we should update the unit tests to use descriptors that contain IPv6 addresses in "or-address" lines.
This will allow us to unit test #17840 and various other tickets.
From #17840:
* nodelist_set_consensus: setting ipv6_preferred correctly based on ClientUseIPv6, ClientPreferIPv6ORPort, and ClientUseIPv4.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/17740Unit Tests for Recent Consensuses2020-06-13T14:51:45ZteorUnit Tests for Recent ConsensusesIt would be great to have unit tests for the functions that return a recent consensus:
Mock:
* networkstatus_get_latest_consensus_by_flavor
Test:
* networkstatus_get_latest_consensus
* networkstatus_get_reasonably_live_consensus
* netw...It would be great to have unit tests for the functions that return a recent consensus:
Mock:
* networkstatus_get_latest_consensus_by_flavor
Test:
* networkstatus_get_latest_consensus
* networkstatus_get_reasonably_live_consensus
* networkstatus_consensus_is_boostrappingTor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/17715Write unit tests for directory_initiate_command_rend2020-06-13T14:51:39ZteorWrite unit tests for directory_initiate_command_rendTo do this:
* Mock connection_connect to do nothing and return 0
* Mock directory_send_command to do nothing
* Mock connection_ap_make_link to return a fake connection
* just return the partner connection that's passed to it?
It would...To do this:
* Mock connection_connect to do nothing and return 0
* Mock directory_send_command to do nothing
* Mock connection_ap_make_link to return a fake connection
* just return the partner connection that's passed to it?
It would be nice to get this done in 0.2.8, but I think it's more likely to be 0.2.9.Tor: unspecifiedhttps://gitlab.torproject.org/legacy/trac/-/issues/17684Simplify directory_get_from_dirserver so it can be unit tested2020-06-13T14:51:27ZteorSimplify directory_get_from_dirserver so it can be unit testedIn #4483, I refactor directory_get_from_dirserver so it can be unit tested.
But there's a series of stubborn nested conditionals in the middle of the function, that also change the values they're checking.
If we simplified this knot of...In #4483, I refactor directory_get_from_dirserver so it can be unit tested.
But there's a series of stubborn nested conditionals in the middle of the function, that also change the values they're checking.
If we simplified this knot of conditions, then it would be much easier to unit test.Tor: unspecified