Page MenuHomePhabricator

util: fix ReadBinaryFile() and WriteBinaryFile() return status
ClosedPublic

Authored by PiRK on Feb 8 2022, 14:21.

Details

Summary

util: fix ReadBinaryFile() returning partial contents

If an error occurs and fread() returns 0 (nothing was read) then the
code before this patch would have returned "success" with a partially
read contents of the file.

https://github.com/bitcoin/bitcoin/pull/20685/commits/8b6e4b3b23027da263d257b342f5d9a53e4032d5

util: fix WriteBinaryFile() claiming success even if error occurred

fclose() is flushing any buffered data to disk, so if it fails then
that could mean that the data was not completely written to disk.

Thus, check if fclose() succeeds and only then claim success from
WriteBinaryFile().

https://github.com/bitcoin/bitcoin/pull/20685/commits/545bc5f81d60fa6ff7c5cc43a2e9eef82f911466

This is a backport of core#20685 [2&3/20]

Depends on D11012

Test Plan

ninja all check-all

eviewers:

Event Timeline

PiRK requested review of this revision.Feb 8 2022, 14:21

Tail of the build log:

[322/512] Building CXX object src/CMakeFiles/util.dir/util/strencodings.cpp.o
[323/512] Building CXX object src/CMakeFiles/util.dir/blockdb.cpp.o
[324/512] Building CXX object src/CMakeFiles/util.dir/util/error.cpp.o
[325/512] Linking C static library src/secp256k1/libsecp256k1.a
[326/512] Building CXX object src/CMakeFiles/util.dir/util/asmap.cpp.o
[327/512] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/arith_uint256.cpp.o
[328/512] Linking C executable src/secp256k1/ecmult-bench
[329/512] Linking C executable src/secp256k1/internal-bench
[330/512] Linking C executable src/secp256k1/sign-bench
[331/512] Linking C executable src/secp256k1/verify-bench
[332/512] Linking C executable src/secp256k1/recover-bench
[333/512] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/uint256.cpp.o
[334/512] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/hash.cpp.o
[335/512] Building CXX object src/CMakeFiles/util.dir/util/sock.cpp.o
[336/512] Building CXX object src/CMakeFiles/util.dir/util/settings.cpp.o
[337/512] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[338/512] Building CXX object src/CMakeFiles/util.dir/util/message.cpp.o
[339/512] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/util/strencodings.cpp.o
[340/512] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/consensus/tx_check.cpp.o
[341/512] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[342/512] Building CXX object src/CMakeFiles/bitcoinconsensus.dir/primitives/transaction.cpp.o
[343/512] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[344/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/context.cpp.o
[345/512] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[346/512] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[347/512] Building CXX object src/CMakeFiles/util.dir/util/time.cpp.o
[348/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[349/512] Building CXX object src/test/CMakeFiles/testutil.dir/util/net.cpp.o
[350/512] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[351/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[352/512] Building CXX object src/CMakeFiles/util.dir/util/system.cpp.o
[353/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[354/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[355/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/bdb.cpp.o
[356/512] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[357/512] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqabstractnotifier.cpp.o
[358/512] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[359/512] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[360/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/sqlite.cpp.o
[361/512] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqnotificationinterface.cpp.o
[362/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[363/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[364/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[365/512] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqutil.cpp.o
[366/512] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/dns.cpp.o
[367/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/salvage.cpp.o
[368/512] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqpublishnotifier.cpp.o
[369/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[370/512] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqrpc.cpp.o
[371/512] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/db.cpp.o
[372/512] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/options.cpp.o
[373/512] Building CXX object src/seeder/CMakeFiles/seeder-base.dir/bitcoin.cpp.o
[374/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[375/512] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[376/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[377/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[378/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
[379/512] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

Failed tests logs:

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

------- Stdout: -------
2022-02-08T14:37:35.626000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220208_143206/abc_p2p_getavaaddr_191
2022-02-08T14:37:38.835000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T14:37:42.366000Z 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/abc_p2p_getavaaddr.py", line 201, in run_test
    self.address_test(maxaddrtosend=3, num_proof=2, num_avanode=8)
  File "/work/test/functional/abc_p2p_getavaaddr.py", line 183, in address_test
    requester = node.add_p2p_connection(AddrReceiver())
  File "/work/test/functional/test_framework/test_node.py", line 701, in add_p2p_connection
    p2p_conn.wait_for_verack()
  File "/work/test/functional/test_framework/p2p.py", line 604, in wait_for_verack
    self.wait_until(test_function, timeout=timeout)
  File "/work/test/functional/test_framework/p2p.py", line 518, in wait_until
    timeout_factor=self.timeout_factor)
  File "/work/test/functional/test_framework/util.py", line 273, in wait_until_helper
    if predicate():
  File "/work/test/functional/test_framework/p2p.py", line 514, in test_function
    assert self.is_connected
AssertionError
2022-02-08T14:37:42.417000Z TestFramework (INFO): Stopping nodes
------- Stderr: -------
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/abc_p2p_getavaaddr.py", line 207, in <module>
    AvaAddrTest().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 426, 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: abc_p2p_getavaaddr.py

needs a rebase after D11012 is fixed

Fabien requested changes to this revision.Feb 8 2022, 19:34
This revision now requires changes to proceed.Feb 8 2022, 19:34
Fabien requested changes to this revision.Feb 9 2022, 11:38
Fabien added inline comments.
src/util/readwritefile.cpp
48 ↗(On Diff #32290)
return fclose(f) == 0;
This revision now requires changes to proceed.Feb 9 2022, 11:38

simplify return statetement

This revision is now accepted and ready to land.Feb 10 2022, 08:30