[tor-bugs] #32088 [Core Tor/Tor]: Proposal 310 - choose guards in sampled order

[Tor Bug Tracker & Wiki]

#32088: Proposal 310 - choose guards in sampled order
-------------------------------------------------+-------------------------
 Reporter:  Jaym                                 |          Owner:  (none)
     Type:  enhancement                          |         Status:
                                                 |  needs_review
 Priority:  High                                 |      Milestone:  Tor:
                                                 |  0.4.4.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-spec prop271 prop310 044-should  |  Actual Points:
Parent ID:                                       |         Points:
 Reviewer:  nickm, asn                           |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by nickm):

 * status:  needs_revision => needs_review


Comment:

 Oh!  It sounds like this is ready for a fresh review then.  I'll put it
 back in needs_review.

 (Yeah, I agree about testing.  It's gotten to the point where when I write
 a test that passes the first time, I am suspicious that it isn't really
 testing the code.)

 As for the tests, it looks like you're getting this compilation error:
 {{{
 x86_64-w64-mingw32-gcc -DHAVE_CONFIG_H -I. -I..
 -DSHARE_DATADIR="\"/mingw64/share\"" -DLOCALSTATEDIR="\"/mingw64/var\""
 -DBINDIR="\"/mingw64/bin\"" -DTOR_UNIT_TESTS -I../src -I../src/ext
 -I../src/ext/trunnel -I../src/trunnel -I../src/ext -Isrc/ext
 -DSHARE_DATADIR="\"/mingw64/share\"" -DLOCALSTATEDIR="\"/mingw64/var\""
 -DBINDIR="\"/mingw64/bin\"" -DTOR_UNIT_TESTS  -DHAVE_MODULE_RELAY=1
 -DHAVE_MODULE_DIRAUTH=1 -DHAVE_MODULE_DIRCACHE=1 -I/mingw64/include
 -IC:/msys64/mingw64/include -L/mingw64/include  -g -O2 -fasynchronous-
 unwind-tables -Wall -fno-strict-aliasing -Werror  -Waddress -Warray-bounds
 -Wdate-time -Wdouble-promotion -Wduplicate-decl-specifier -Wduplicated-
 cond -Wenum-conversion -Wextra -Wfloat-conversion -Wignored-attributes
 -Wimplicit-fallthrough -Winit-self -Wlogical-op -Wmissing-field-
 initializers -Wmissing-format-attribute -Wmissing-noreturn
 -Wnormalized=nfkc -Wnull-dereference -Woverlength-strings -Woverride-init
 -Wshadow -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-
 value -Wshift-overflow=2 -Wsizeof-array-argument -Wstrict-overflow=1
 -Wstring-compare -Wsuggest-attribute=format -Wsuggest-attribute=noreturn
 -Wswitch-bool -Wsync-nand -Wtrampolines -Wunused-but-set-parameter
 -Wunused-but-set-variable -Wunused-const-variable=2 -Wunused-local-
 typedefs -Wvariadic-macros  -W -Wfloat-equal -Wundef -Wpointer-arith
 -Wstrict-prototypes -Wmissing-prototypes -Wwrite-strings -Wredundant-decls
 -Wchar-subscripts -Wcomment -Wformat=2 -Wwrite-strings -Wnested-externs
 -Wbad-function-cast -Wswitch-enum -Waggregate-return -Wpacked -Wunused
 -Wunused-parameter  -Wold-style-definition -Wmissing-declarations -MT
 src/test/test-test_geoip.o -MD -MP -MF src/test/.deps/test-test_geoip.Tpo
 -c -o src/test/test-test_geoip.o `test -f 'src/test/test_geoip.c' || echo
 '../'`src/test/test_geoip.c
 bash.exe : In file included from ../src/lib/container/smartlist.h:18,
 At line:2 char:5
 +     & $commandPath $args 2>&1
 +     ~~~~~~~~~~~~~~~~~~~~~~~~~
     + CategoryInfo          : NotSpecified: (In file
 include...smartlist.h:18,:String) [], RemoteException
     + FullyQualifiedErrorId : NativeCommandError

                  from ../src/core/or/or.h:29,
                  from ../src/test/test_entrynodes.c:14:
 ../src/test/test_entrynodes.c: In function
 'test_entry_guard_correct_cascading_order':
 ../src/lib/smartlist_core/smartlist_core.h:40:55: error:
 'old_primary_guards' may be used uninitialized in this function [-Werror
 =maybe-uninitialized]
    40 | #define smartlist_free(sl) FREE_AND_NULL(smartlist_t,
 smartlist_free_, (sl))
       |
 ^~~~~~~~~~~~~~~
 ../src/test/test_entrynodes.c:1825:16: note: 'old_primary_guards' was
 declared here
  1825 |   smartlist_t *old_primary_guards = smartlist_new();
       |                ^~~~~~~~~~~~~~~~~~
 mv -f src/test/.deps/test-test_geoip.Tpo src/test/.deps/test-test_geoip.Po
 }}}

 The issue here is that all of the `tt_...()` macros will run `goto done;`
 on failure.  The `done` block of
 `test_entry_guard_correct_cascading_order` is calling
 `smartlist_free(old_primary_guards)`... but that variable isn't actually
 declared until half-way through the function.  You can fix this error by
 declaring it earlier.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/32088#comment:30>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online