Page MenuHomePhabricator
Differential D10748

Add bounds checking to seeder arguments with integer base types
ClosedPublic
Actions

Authored by thonkle on Dec 31 2021, 23:31.

Details

Reviewers
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC4885d0a59315: Add bounds checking to seeder arguments with integer base types
Summary

The seeder does not implement bounds checking for integer typed arguments.
In most cases, no error or warning is presented to the user that indicates the seeder
is not operating correctly.
In some cases, strange behavior is produced (for example, negative port values underflow).

Test Plan
ninja bitcoin-seeder check-seeder

Listening port is still correct (verify with sudo netstat -pln | grep seeder):

./src/seeder/bitcoin-seeder -host=seeder.status.cash -ns=localhost -mbox=thonkle@protonmail.com -port=53
./src/seeder/bitcoin-seeder -host=seeder.status.cash -ns=localhost -mbox=thonkle@protonmail.com -port=5555

Use ls /proc/$(pgrep seeder)/task/ | wc -l to verify number of threads:

./src/seeder/bitcoin-seeder -host=seeder.status.cash -ns=localhost -mbox=thonkle@protonmail.com -port=5555  # expect 104
./src/seeder/bitcoin-seeder -host=seeder.status.cash -ns=localhost -mbox=thonkle@protonmail.com -port=5555 -threads=1  # expect 9
./src/seeder/bitcoin-seeder -host=seeder.status.cash -ns=localhost -mbox=thonkle@protonmail.com -port=5555 -dnsthreads=1  # expect 101
./src/seeder/bitcoin-seeder -host=seeder.status.cash -ns=localhost -mbox=thonkle@protonmail.com -port=5555 -threads=2 -dnsthreads=2  # expect 8

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

thonkle created this revision.Dec 31 2021, 23:31
Owners added a reviewer: Restricted Owners Package.Dec 31 2021, 23:31
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 31 2021, 23:31
thonkle requested review of this revision.Dec 31 2021, 23:31
Harbormaster completed remote builds in B17768: Diff 31596.Dec 31 2021, 23:46
Fabien requested changes to this revision.Jan 3 2022, 07:35
Fabien added a subscriber: Fabien.

This deserves a unit test.
You can just call ParseCommandLine() with various values to check the behavior.

Also the port number can have an upper bound as well.

This revision now requires changes to proceed.Jan 3 2022, 07:35
thonkle mentioned this in D10790: Make CDnsSeedOpts unit testable.Jan 7 2022, 19:18
thonkle added a comment.Jan 7 2022, 19:20
In D10748#241247, @Fabien wrote:

This deserves a unit test.
You can just call ParseCommandLine() with various values to check the behavior.

Also the port number can have an upper bound as well.

You are right but refactor is needed first: D10790

thonkle mentioned this in rABCbca5c5ee6ca6: Make CDnsSeedOpts unit testable.Jan 11 2022, 17:45
thonkle updated this revision to Diff 32062.Jan 28 2022, 15:28

Add unit tests for integer args bounds

thonkle edited the test plan for this revision. (Show Details)Jan 28 2022, 15:29
thonkle added inline comments.Jan 28 2022, 15:32
src/seeder/test/options_tests.cpp
89 ↗(On Diff #32062)

This behavior is undocumented. I will document this in a separate patch, and I am starting with upstream in the node software first: https://github.com/bitcoin/bitcoin/pull/24116

Harbormaster completed remote builds in B18063: Diff 32062.Jan 28 2022, 15:53
Fabien added inline comments.Jan 28 2022, 15:55
src/seeder/options.cpp
47 ↗(On Diff #32062)

Why is 65535 prohibited ?

thonkle updated this revision to Diff 32068.Jan 28 2022, 22:22

Fix port boundary at 65535

Harbormaster completed remote builds in B18067: Diff 32068.Jan 28 2022, 22:33
teamcity added a comment.Jan 28 2022, 22:33
Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: wallet_multiwallet.py ======

------- Stdout: -------
2022-01-28T22:27:30.818000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220128_222728/wallet_multiwallet_0
2022-01-28T22:27:57.839000Z TestFramework (INFO): Check for per-wallet settxfee call
2022-01-28T22:27:57.855000Z TestFramework (INFO): Test dynamic wallet loading
2022-01-28T22:27:58.980000Z TestFramework (INFO): Load first wallet
2022-01-28T22:27:59.023000Z TestFramework (INFO): Load second wallet
2022-01-28T22:27:59.039000Z TestFramework (INFO): Concurrent wallet loading
2022-01-28T22:27:59.302000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/work/test/functional/wallet_multiwallet.py", line 288, in run_test
    assert_equal(got_loading_error, True)
  File "/work/test/functional/test_framework/util.py", line 60, in assert_equal
    for arg in (thing1, thing2) + args)))
AssertionError: not(False == True)
2022-01-28T22:27:59.353000Z TestFramework (INFO): Stopping nodes
------- Stderr: -------
Exception in thread Thread-3:
Traceback (most recent call last):
  File "/work/test/functional/test_framework/authproxy.py", line 117, in _request
    return self._get_response()
  File "/work/test/functional/test_framework/authproxy.py", line 187, in _get_response
    http_response = self.__conn.getresponse()
  File "/usr/lib/python3.7/http/client.py", line 1352, in getresponse
    response.begin()
  File "/usr/lib/python3.7/http/client.py", line 310, in begin
    version, status, reason = self._read_status()
  File "/usr/lib/python3.7/http/client.py", line 271, in _read_status
    line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1")
  File "/usr/lib/python3.7/socket.py", line 589, in readinto
    return self._sock.recv_into(b)
ConnectionResetError: [Errno 104] Connection reset by peer

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.7/threading.py", line 917, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.7/threading.py", line 865, in run
    self._target(*self._args, **self._kwargs)
  File "/work/test/functional/wallet_multiwallet.py", line 32, in test_load_unload
    node.loadwallet(name)
  File "/work/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 161, in __call__
    'POST', self.__url.path, postdata.encode('utf-8'))
  File "/work/test/functional/test_framework/authproxy.py", line 122, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1030, in _send_output
    self.send(msg)
  File "/usr/lib/python3.7/http/client.py", line 970, in send
    self.connect()
  File "/usr/lib/python3.7/http/client.py", line 942, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/lib/python3.7/socket.py", line 727, in create_connection
    raise err
  File "/usr/lib/python3.7/socket.py", line 716, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

Exception in thread Thread-2:
Traceback (most recent call last):
  File "/work/test/functional/test_framework/authproxy.py", line 117, in _request
    return self._get_response()
  File "/work/test/functional/test_framework/authproxy.py", line 187, in _get_response
    http_response = self.__conn.getresponse()
  File "/usr/lib/python3.7/http/client.py", line 1352, in getresponse
    response.begin()
  File "/usr/lib/python3.7/http/client.py", line 310, in begin
    version, status, reason = self._read_status()
  File "/usr/lib/python3.7/http/client.py", line 279, in _read_status
    raise RemoteDisconnected("Remote end closed connection without"
http.client.RemoteDisconnected: Remote end closed connection without response

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.7/threading.py", line 917, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.7/threading.py", line 865, in run
    self._target(*self._args, **self._kwargs)
  File "/work/test/functional/wallet_multiwallet.py", line 32, in test_load_unload
    node.loadwallet(name)
  File "/work/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 161, in __call__
    'POST', self.__url.path, postdata.encode('utf-8'))
  File "/work/test/functional/test_framework/authproxy.py", line 122, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1030, in _send_output
    self.send(msg)
  File "/usr/lib/python3.7/http/client.py", line 970, in send
    self.connect()
  File "/usr/lib/python3.7/http/client.py", line 942, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/lib/python3.7/socket.py", line 727, in create_connection
    raise err
  File "/usr/lib/python3.7/socket.py", line 716, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

Exception in thread Thread-1:
Traceback (most recent call last):
  File "/work/test/functional/test_framework/authproxy.py", line 117, in _request
    return self._get_response()
  File "/work/test/functional/test_framework/authproxy.py", line 187, in _get_response
    http_response = self.__conn.getresponse()
  File "/usr/lib/python3.7/http/client.py", line 1352, in getresponse
    response.begin()
  File "/usr/lib/python3.7/http/client.py", line 310, in begin
    version, status, reason = self._read_status()
  File "/usr/lib/python3.7/http/client.py", line 279, in _read_status
    raise RemoteDisconnected("Remote end closed connection without"
http.client.RemoteDisconnected: Remote end closed connection without response

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.7/threading.py", line 917, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.7/threading.py", line 865, in run
    self._target(*self._args, **self._kwargs)
  File "/work/test/functional/wallet_multiwallet.py", line 33, in test_load_unload
    node.unloadwallet(name)
  File "/work/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 161, in __call__
    'POST', self.__url.path, postdata.encode('utf-8'))
  File "/work/test/functional/test_framework/authproxy.py", line 122, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1030, in _send_output
    self.send(msg)
  File "/usr/lib/python3.7/http/client.py", line 970, in send
    self.connect()
  File "/usr/lib/python3.7/http/client.py", line 942, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/lib/python3.7/socket.py", line 727, in create_connection
    raise err
  File "/usr/lib/python3.7/socket.py", line 716, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

Traceback (most recent call last):
  File "/work/test/functional/test_framework/authproxy.py", line 116, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1069, in _send_output
    self.send(chunk)
  File "/usr/lib/python3.7/http/client.py", line 991, in send
    self.sock.sendall(data)
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/work/test/functional/wallet_multiwallet.py", line 470, in <module>
    MultiWalletTest().main()
  File "/work/test/functional/test_framework/test_framework.py", line 152, in main
    exit_code = self.shutdown()
  File "/work/test/functional/test_framework/test_framework.py", line 280, in shutdown
    self.stop_nodes()
  File "/work/test/functional/test_framework/test_framework.py", line 514, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/work/test/functional/test_framework/test_node.py", line 423, in stop_node
    self.stop(wait=wait)
  File "/work/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 161, in __call__
    'POST', self.__url.path, postdata.encode('utf-8'))
  File "/work/test/functional/test_framework/authproxy.py", line 122, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1030, in _send_output
    self.send(msg)
  File "/usr/lib/python3.7/http/client.py", line 970, in send
    self.connect()
  File "/usr/lib/python3.7/http/client.py", line 942, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/lib/python3.7/socket.py", line 727, in create_connection
    raise err
  File "/usr/lib/python3.7/socket.py", line 716, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

Each failure log is accessible here:
Bitcoin ABC functional tests: wallet_multiwallet.py

Fabien accepted this revision.Jan 29 2022, 09:21
This revision is now accepted and ready to land.Jan 29 2022, 09:21
This revision was landed with ongoing or failed builds.Jan 31 2022, 15:18
Closed by commit rABC4885d0a59315: Add bounds checking to seeder arguments with integer base types (authored by thonkle). · Explain Why
This revision was automatically updated to reflect the committed changes.
thonkle added a commit: rABC4885d0a59315: Add bounds checking to seeder arguments with integer base types.

Revision Contents
Changeset List

PathSize
src/
seeder/
20 lines
test/
73 lines

Diff 32082

View Options

src/seeder/options.cpp

Loading...
View Options

src/seeder/test/options_tests.cpp

Loading...