Call node_get_all_orports() less from node_is_a_configured_bridge()

According to our profile, this function accounts for a huge amount of the total calls to malloc() that we do. Although I don't think it accounts for much allocation at any given time, it is probably slowing us down.

changed milestone to %Tor: 0.3.5.x-final

added 035-roadmap-master 035-triaged-in-20180711 component::core tor/tor milestone::Tor: 0.3.5.x-final owner::rl1987 parent::26630 priority::medium resolution::implemented reviewer::teor severity::normal sponsor::8 status::closed type::enhancement labels

Trac:
Status: new to accepted
Owner: N/A to rl1987

The patch could be something like this: https://github.com/rl1987/tor/commit/cf9a7a8e8f82bc769220f5859ef00e3e9a796654

However, for some reason this makes entrynodes/node_filter fail. Couldn't figure out yet. Can anyone spot the key difference between old code and new code?

Okay, I got it done and ready for review. https://github.com/torproject/tor/pull/293

Trac:
Status: accepted to needs_review

This branch fails Linux gcc with:

gcc -std=gnu99 -DHAVE_CONFIG_H -I.  -I./src -I./src/ext -I./src/ext/trunnel -I./src/trunnel -I./src/ext -Isrc/ext -DSHARE_DATADIR="\"/usr/local/share\"" -DLOCALSTATEDIR="\"/usr/local/var\"" -DBINDIR="\"/usr/local/bin\"" -DTOR_UNIT_TESTS  -DHAVE_MODULE_DIRAUTH=1  -ftrapv -fsanitize=address      -g -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wstack-protector --param ssp-buffer-size=1 -fPIE -fno-omit-frame-pointer -fasynchronous-unwind-tables -Wall -fno-strict-aliasing -Waddress -Warray-bounds -Wdouble-promotion -Wextra -Winit-self -Wlogical-op -Wmissing-field-initializers -Wmissing-format-attribute -Wmissing-noreturn -Wnormalized=nfkc -Woverlength-strings -Woverride-init -Wshadow -Wstrict-overflow=1 -Wsuggest-attribute=format -Wsuggest-attribute=noreturn -Wsync-nand -Wtrampolines -Wunused-but-set-parameter -Wunused-but-set-variable -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 -Werror -MT src/feature/dircache/src_core_libtor_app_testing_a-directory.o -MD -MP -MF src/feature/dircache/.deps/src_core_libtor_app_testing_a-directory.Tpo -c -o src/feature/dircache/src_core_libtor_app_testing_a-directory.o `test -f 'src/feature/dircache/directory.c' || echo './'`src/feature/dircache/directory.c
src/feature/client/bridges.c: In function ‘bridge_exists_with_ipv4h_addr_and_port’:
src/feature/client/bridges.c:291:3: error: missing initializer for field ‘family’ of ‘tor_addr_t’ [-Werror=missing-field-initializers]
   tor_addr_t node_ipv4 = {};
   ^
In file included from ./src/core/or/or.h:51:0,
                 from src/feature/client/bridges.c:16:
./src/lib/net/address.h:69:15: note: ‘family’ declared here
   sa_family_t family;
               ^
cc1: all warnings being treated as errors

I believe the correct syntax for zero-initialising any number of struct members is:

  tor_addr_t node_ipv4 = {0};

But it's slightly more correct to do:

  tor_addr_t node_ipv4;
  tor_addr_make_unspec(&node_ipv4, AF_INET);

Trac:
Status: needs_review to needs_revision

Oh, the failed CI is here:

• https://travis-ci.org/torproject/tor/builds/420815262

Replying to teor:

But it's slightly more correct to do: {{{ tor_addr_t node_ipv4; tor_addr_make_unspec(&node_ipv4, AF_INET); }}}

Could also just move the declaration inside the if statement, right before the tor_addr_from_ipv4h() call, right?

Trac:
Username: cyberpunks

Interesting - it compiled alright in my Vagrant box. Pushed some fixup commits that removed the offending syntax.

Trac:
Status: needs_revision to needs_review

Thanks! Looks fine to me.

Trac:
Status: needs_review to merge_ready

I don't think this will actually return the same results as the old function.

In the case when the bridge has no identity set, but where the node does have an identity (*), this code will always return "false" -- but the old code would check the addresses in that case.

There may be other cases when the outputs are different; I'm not sure that I spotted them all.

(*) I think the node will always have an identity, in fact.

Trac:
Status: merge_ready to needs_revision

Here's the behavior that I think we had before, and which I think we want:

• A node will match every bridge_info_t with the same identity.
• A node will match every bridge_info_t with no identity set, if that bridge_info_t has the same address in the routerinfo, the routerstatus, or the microdescriptor.

Pushed one more commit that hopefully restores the old behaviour: https://github.com/rl1987/tor/commit/2e142195a0180bdd2d9354c14e53f03180359840

Please review.

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to teor

Trac:
Type: defect to enhancement

To test this, when I squashed the branch, I tried taking only the unit tests, with none of the bridges.c changes, and making sure that they passed on the old bridges.c code as well as the new. They did! So, that's good.

But then I was worried that you hadn't tested the case where a bridge had a matching address but a mismatched digest. In fact, I thought it wouldn't work, since I had forgotten how get_configured_bridge_by_addr_port_digest actually behaved... So I added a test for that too -- and the code still passes before and after.

Nicely done! I think this code is correct.

I'm merging bug27224_take2_squashed to master now. It's the same as your branch, squashed and rearranged, with an extra test.

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed

moved to tpo/core/tor#27224 (closed)