Page MenuHomePhabricator
Differential D11013

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

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

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCc577087f7627: util: fix ReadBinaryFile() and WriteBinaryFile() return status
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:

Diff Detail

Event Timeline

PiRK created this revision.Feb 8 2022, 14:21
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 8 2022, 14:21
PiRK requested review of this revision.Feb 8 2022, 14:21
teamcity edited the summary of this revision. (Show Details)Feb 8 2022, 14:22
PiRK updated this revision to Diff 32274.Feb 8 2022, 14:23

add missing braces

PiRK added a child revision: D11014: net: check for invalid socket earlier and get the bind address earlier in CConnman::AcceptConnection().Feb 8 2022, 14:27
Harbormaster completed remote builds in B18196: Diff 32273.Feb 8 2022, 14:31
teamcity added a comment.Feb 8 2022, 14:31
Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

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
teamcity added a comment.Feb 8 2022, 14:39
Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

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

Harbormaster completed remote builds in B18197: Diff 32274.Feb 8 2022, 14:56
Fabien added a subscriber: Fabien.Feb 8 2022, 19:29

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
PiRK updated this revision to Diff 32290.Feb 9 2022, 08:04
PiRK mentioned this in D11012: util: extract {Read,Write}BinaryFile() to its own files.

rebase

Harbormaster completed remote builds in B18213: Diff 32290.Feb 9 2022, 08:26
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
PiRK updated this revision to Diff 32306.Feb 9 2022, 13:46

simplify return statetement

Harbormaster completed remote builds in B18220: Diff 32306.Feb 9 2022, 14:02
Fabien accepted this revision.Feb 10 2022, 08:30
This revision is now accepted and ready to land.Feb 10 2022, 08:30
Closed by commit rABCc577087f7627: util: fix ReadBinaryFile() and WriteBinaryFile() return status (authored by Vasil Dimov <vd@FreeBSD.org>, committed by PiRK). · Explain WhyFeb 10 2022, 09:13
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABCc577087f7627: util: fix ReadBinaryFile() and WriteBinaryFile() return status.

Revision Contents
Changeset List

PathSize
src/
util/
13 lines

Diff 32273

View Options

src/util/readwritefile.cpp

Loading...