Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: fix incorrect less operator for top/i2p addresses #6285

Merged
merged 1 commit into from Mar 27, 2020

Conversation

@ahook
Copy link

ahook commented Jan 8, 2020

The less(lhs, rhs) functions for both i2p and tor addresses do not work as expected. The logic should only be testing the ports if the hosts are equal (if currently falls back to ports if lhs.host >= rhs.host).

A concrete example:
let addrA = aaaaaaaaaaaaaaaa.onion:443
and addrZ =zzzzzzzzzzzzzzzz.onion:80

less(addrA, addrZ) returns true (since strcmp(addrA.host, addrZ.host) < 0)
less(addrZ, addrA) returns true (since strcmp(addrZ.host, addrA.host) > 0 and addrZ.port < addrA.port)

@vtnerd

This comment has been minimized.

Copy link
Contributor

vtnerd commented Jan 12, 2020

Changes to unit tests?

@ahook

This comment has been minimized.

Copy link
Author

ahook commented Jan 14, 2020

Changes to unit tests?

Will do! I'll push some unit tests in the next day or two

@ahook ahook force-pushed the ahook:akh_tor_i2p_less branch from 9c6f6b0 to d0641b4 Jan 19, 2020
@ahook

This comment has been minimized.

Copy link
Author

ahook commented Jan 19, 2020

Changes to unit tests?

Added unit tests (and squashed/rebased). There were no unit tests for i2p so I also made i2p versions of all the tor test cases.

Running these tests without the src changes leaves you with failures:

[----------] 11 tests from tor_address
[ RUN      ] tor_address.valid
/home/ahook/codebase/ahook/monero/tests/unit_tests/net.cpp:207: Failure
Value of: address2.less(address3)
  Actual: true
Expected: false
[  FAILED  ] tor_address.valid (0 ms)
[----------] 11 tests from tor_address (7 ms total)

[----------] 9 tests from i2p_address
[ RUN      ] i2p_address.valid
/home/ahook/codebase/ahook/monero/tests/unit_tests/net.cpp:662: Failure
Value of: address2.less(address3)
  Actual: true
Expected: false
[----------] 9 tests from i2p_address (6 ms total)

Running with the src changes in place resolves the failures:

[----------] 11 tests from tor_address
[ RUN      ] tor_address.invalid
[       OK ] tor_address.invalid (6 ms)
[----------] 11 tests from tor_address (6 ms total)

[----------] 4 tests from get_network_address
[ RUN      ] get_network_address.i2p
[       OK ] get_network_address.i2p (6 ms)
[----------] 4 tests from get_network_address (14 ms total)

[----------] 9 tests from i2p_address
[ RUN      ] i2p_address.constants
[       OK ] i2p_address.constants (0 ms)
[ RUN      ] i2p_address.invalid
[       OK ] i2p_address.invalid (6 ms)
[ RUN      ] i2p_address.unblockable_types
[       OK ] i2p_address.unblockable_types (0 ms)
[ RUN      ] i2p_address.valid
[       OK ] i2p_address.valid (0 ms)
[ RUN      ] i2p_address.generic_network_address
[       OK ] i2p_address.generic_network_address (0 ms)
[ RUN      ] i2p_address.epee_serializev_b32
[       OK ] i2p_address.epee_serializev_b32 (0 ms)
[ RUN      ] i2p_address.epee_serialize_unknown
[       OK ] i2p_address.epee_serialize_unknown (0 ms)
[ RUN      ] i2p_address.boost_serialize_b32
[       OK ] i2p_address.boost_serialize_b32 (0 ms)
[ RUN      ] i2p_address.boost_serialize_unknown
[       OK ] i2p_address.boost_serialize_unknown (0 ms)
[----------] 9 tests from i2p_address (6 ms total)
@vtnerd
vtnerd approved these changes Mar 7, 2020
@Snipa22 Snipa22 merged commit 7d4a93f into monero-project:master Mar 27, 2020
1 of 5 checks passed
1 of 5 checks passed
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.