Make (Read/Write)BinaryFile work with char vector, use AutoFile #29229

Sjors · 2024-01-11T15:10:56Z

ReadBinaryFile and WriteBinaryFile current work with std::string. This PR adds support for std::vector<unsigned char>>.

It also uses AutoFile now.

This is [update: probably not] used in #28983 to store the static key for the Template Provider, in a manner very similar to how we store the Tor v3 and i2p key. However it made no sense to me to store a CKey as plain text. See commit "Persist static key for Template Provider" for how it's used.

It uses a template and leverages the fact that both std::string and std::vector<unsigned char>> have an insert() method that can take a char array.

The unsigned char support is not used in this PR, but tests do cover it.

DrahtBot · 2024-01-11T15:11:00Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors · 2024-01-11T15:15:16Z

cc @vasild

src/util/readwritefile.h

Sjors · 2024-01-11T15:51:52Z

Added extern template ... to the header to prevent "implicit instantiation of undefined template" error on the "no wallet, libbitcoinkernel" CI (https://cirrus-ci.com/task/6254856449556480)

That error doesn't happen on my machine running macOS clang 15, nor on Ubuntu gcc 13.2 - maybe a specific configure warning flag?

Update: that didn't work, still not sure how to reproduce.

 util/readwritefile.cpp:23:84: error: implicit instantiation of undefined template 'std::vector<unsigned char>'
        const size_t n = fread(buffer, 1, std::min(sizeof(buffer), maxsize - output.size()), f);

DrahtBot · 2024-01-11T15:51:55Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20390186031}

Sjors · 2024-01-12T08:26:32Z

I switched to using std::optional as the return type. It happily compiles on my end, we'll see what CI thinks...

Sjors · 2024-01-12T08:47:01Z

Had to #include <vector> to please clang13 (should have included it anyway since it's used).

vasild · 2024-01-18T15:12:43Z

However it made no sense to me to store a CKey as plain text

I guess that by "plain text" here you mean std::string, right? std::string can store arbitrary non-ASCII characters, including '\0', so it is technically ok to use it for binary data.

More relevant in this case is that CKey stores sensitive data and takes care to wipe it from memory when freed. In #28983 Read/WriteBinaryData() is used in a way that defeats that - the sensitive data will be copied to a temporary variable (ordinary std::vector) for IO. Can we change Read/WriteBinaryData() to operate directly on CKey in such a way that data goes directly from CKey to the disk?

src/Makefile.test.include

src/util/readwritefile.h

maflcko · 2024-01-18T15:19:11Z

Can we change Read/WriteBinaryData() to operate directly on CKey in such a way that data goes directly from CKey to the disk?

I haven't looked in detail, but writing bytes to a file can be achieved with one line of code:

CAutoFile{...} << Span{data};

Sjors · 2024-01-22T09:11:09Z

I like the idea of operating on CKey directly. I'll try to add that, though not sure how to implement that securely.

The keys used in #28983 are not as important as wallet keys, but if we add a generic method to store a CKey on disk, then it should do so securely - so that future devs using that function don't shoot themselves in the foot. At least the current std::vector<unsigned char>> does not pretend to be secure.

Sjors · 2024-01-22T09:15:59Z

I guess that by "plain text" here you mean std::string, right? std::string can store arbitrary non-ASCII characters, including '\0', so it is technically ok to use it for binary data.

I think the way WriteBinaryFile was initially used with Tor was that we parse the JSON returned by the Tor daemon and store the PrivateKey key field (UTF8 encoced?) in a file.

Sjors · 2024-01-22T09:20:46Z

I haven't looked in detail, but writing bytes to a file can be achieved with one line of code

That would be a nice simplification. Almost (?) to the point of not needing these helper functions.

maflcko · 2024-01-22T13:20:11Z

I haven't looked in detail, but writing bytes to a file can be achieved with one line of code

That would be a nice simplification. Almost (?) to the point of not needing these helper functions.

WriteBinaryFile is unused right now either way outside of tests, so I guess this could be removed regardless, as future code can just use the in-line one-liner?

Sjors · 2024-01-23T08:38:10Z

@maflcko WriteBinaryFile is used by Tor and I2P to cache the service private key.

But I might still close this PR if all that's needed is one-liner.

Sjors · 2024-01-23T09:53:08Z

It's not quite a one-liner because you still need to open a close a FILE and deal with various errors. So instead I modified [Write/Write]BinaryFile to use AutoFile instead of fwrite / fread.

However, AutoFile{f} >> output only returns a fraction of the file in the test...

maflcko · 2024-01-23T10:03:37Z

However, AutoFile{f} >> output only returns a fraction of the file in the test...

(De)serialization of vectors or strings assumes the run-time length to be encoded first. Only arrays and spans assume no length, because it is assumed to be known at compile-time.

DrahtBot · 2024-01-23T10:32:44Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20764871422}

Sjors · 2024-01-23T10:37:47Z

Ok, fixed that issue by first getting the file size and then resizing the std::vector / std::string.

That doesn't generalise nicely to known-size things like CKey. The maxsize argument also makes no sense for things with known size. So I might make a separate method for that. Which in turn makes this PR just a refactor with unused, but tested, support for std::vector<unsigned char>.

(this is ready for review even without CKey support, which I'll add in a separate commit and/or PR when I get to it)

Sjors · 2024-01-23T11:01:04Z

I tried to find some existing code that could use the std::vector<unsigned char> variant. Didn't find it at first glance. It seems we almost always know what size to expect (per object).

So that might be a good reason to kill that variant and only support loading an std::string of unknown size for now.

maflcko · 2024-01-23T11:33:54Z

src/util/readwritefile.cpp

+    std::FILE *f = fsbridge::fopen(filename, "wb");
+    if (f == nullptr) return false;
+    try {
+        AutoFile{f} << Span{data};


Suggested change

std::FILE *f = fsbridge::fopen(filename, "wb");

if (f == nullptr) return false;

try {

AutoFile{f} << Span{data};

try {

AutoFile{fsbridge::fopen(filename, "wb")} << Span{data};

nit: Can be written shorter

Took me a while to wrap my head around the various calls involved, but I guess the nullptr check is handled in read(), which is called by the various ser_read... functions in seralize.h, which is called by the Unserialize implementations, which is called by <<. (presumably the same for writing)

On the read side I'm keeping the explicit nullptr check for now, so I don't have to catch fs::file_size failure.

At the point WriteBinaryFile is so small you might as well have the called do the try - catch. But might as well keep if around as long as ReadBinaryFile can't be made smaller.

ReadBinaryFile and WriteBinaryFile currently work with std::string. This commit adds support for std::vector<unsigned char>>. It uses a template and leverages the fact that both std::string and std::vector<unsigned char>> have an insert() method that can take a char array. Also switch to using AutoFile.

Sjors · 2024-01-23T13:52:18Z

I opened #29295 for CKey.

I also refactored #28983 to use AutoFile directly, see Sjors#32. So I no longer need this PR myself, but happy to continue it.

vasild · 2024-01-23T15:40:40Z

src/util/readwritefile.cpp

+    size_t file_size = fs::file_size(filename);
+    output.resize(std::min(file_size, maxsize));
+    try {
+        AutoFile{f} >> Span{output};


I liked more the previous version which called fread(3) here. It was simple stupid. This >> is now hard to follow, especially given that it depends on T. For vector it ends up calling AutoFile::detail_fread(). It does not check whether ferror(3) occurred.

For vector it ends up calling AutoFile::detail_fread(). It does not check whether ferror(3) occurred.

It does. If the return value of detail_fread is not output.size(), operator>> will fail.

It does not check whether ferror(3) occurred.

It does.

Where? There is no ferror(3) call. From the man page: "The function fread() does not distinguish between end-of-file and error, and callers must use feof(3) and ferror(3) to determine which occurred."

If the return value of detail_fread is not output.size(), operator>> will fail.

Yes, but it can be equal to the desired size under two conditions: eof, or error. The previous code distinguished between both.

If the return value of detail_fread is not output.size(), operator>> will fail.

Yes, but it can be equal to the desired size under two conditions: eof, or error.

No, the eof-error would only be raised if read past the desired size, not to it. Unless I am missing something?

I am asking, because if there was a bug, it should be fixed, or at least an issue should be filed.

That "must" in the man page is pretty clear: "callers must use feof(3) and ferror(3) to determine which occurred". A ferror(3) check can't hurt and it is better to have an extra check that always returns "no error" than a missing check, failing to detect an IO error. The previous code was doing that - a dumb fread(3) followed by an unconditional ferror(3).

Yes, it is called to determine which error occurred, see

bitcoin/src/streams.cpp

Line 27 in e69796c

throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed");

Again, if there is a bug in the current code in master, it should be fixed.

I would extend the check to:

void AutoFile::read(Span<std::byte> dst) { - if (detail_fread(dst) != dst.size()) { + if (detail_fread(dst) != dst.size() || std::ferror(m_file)) { throw std::ios_base::failure(feof() ? "AutoFile::read: end of file" : "AutoFile::read: fread failed");

if there is a bug in the current code in master, it should be fixed.

Done in #29307

Glad something useful came out of this PR :-)

vasild · 2024-01-23T15:48:11Z

src/util/readwritefile.cpp

-    }
-    if (fclose(f) != 0) {
+    try {
+        AutoFile{fsbridge::fopen(filename, "wb")} << Span{data};


Same as above, the dumb version was easier to follow. The new one does not check whether fclose(3) failed, but it should. I think that is a serious deficiency in AutoFile itself.

fwrite(3) may succeed, but if a subsequent fclose(3) fails we should consider the data did not make it safely to disk and that the file is corrupted (fclose(3) writes any buffered data to disk using fflush(3), so a failure at fclose(3) is as bad as failure at fwrite(3)).

I think that is a serious deficiency in AutoFile itself

Logged as #29307

@vasild I would also prefer to fix things in AutoFile, but perhaps add a few comments to explain what happens under the hood?

This could happen:

fwrite() succeeds, returns ok the caller, some of the data is buffered in the OS and not yet on disk

fclose() is called, it tries to fflush() the buffered data to disk but fails due to IO error. The caller ignores the error returned by fclose()

the program continues with the wrong assumption that the data is safely on disk

That's not good. I guess we also don't want to sync to disk, and block for that to complete, for every field that's >>'d to a file though.

Right. That's why I don't see a good solution. It is a design issue with AutoFile to flush/close from the destructor which can't signal the failure to the caller.

achow101 · 2024-04-09T15:29:38Z

The PR didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open PRs.

Closing due to lack of interest.

Sjors mentioned this pull request Jan 11, 2024

Stratum v2 Template Provider (take 2) #28983

Closed

30 tasks

maflcko reviewed Jan 11, 2024

View reviewed changes

src/util/readwritefile.h Outdated Show resolved Hide resolved

Sjors force-pushed the 2024/01/rw-binary-file branch from 634f29f to 9b7be63 Compare January 11, 2024 15:51

DrahtBot added the CI failed label Jan 11, 2024

Sjors force-pushed the 2024/01/rw-binary-file branch 2 times, most recently from 18d18ac to 257b14c Compare January 12, 2024 08:26

Sjors force-pushed the 2024/01/rw-binary-file branch from 257b14c to 21618ea Compare January 12, 2024 08:39

DrahtBot removed the CI failed label Jan 12, 2024

DrahtBot mentioned this pull request Jan 12, 2024

kernel: Streamline util library #29015

Open

DrahtBot added the CI failed label Jan 15, 2024

Sjors force-pushed the 2024/01/rw-binary-file branch from 21618ea to ac5a83b Compare January 15, 2024 16:52

DrahtBot removed the CI failed label Jan 15, 2024

vasild reviewed Jan 18, 2024

View reviewed changes

src/Makefile.test.include Show resolved Hide resolved

src/util/readwritefile.h Outdated Show resolved Hide resolved

Sjors force-pushed the 2024/01/rw-binary-file branch from ac5a83b to 7eb6fa4 Compare January 23, 2024 09:50

Sjors marked this pull request as draft January 23, 2024 09:51

Sjors force-pushed the 2024/01/rw-binary-file branch from 7eb6fa4 to 5e82c45 Compare January 23, 2024 10:32

DrahtBot added the CI failed label Jan 23, 2024

Sjors marked this pull request as ready for review January 23, 2024 10:39

Sjors changed the title ~~Make (Read/Write)BinaryFile work with char vector~~ Make (Read/Write)BinaryFile work with char vector, use AutoFile Jan 23, 2024

Sjors force-pushed the 2024/01/rw-binary-file branch from 5e82c45 to e4d0e3d Compare January 23, 2024 10:45

maflcko reviewed Jan 23, 2024

View reviewed changes

DrahtBot removed the CI failed label Jan 23, 2024

Sjors force-pushed the 2024/01/rw-binary-file branch from e4d0e3d to f9b134e Compare January 23, 2024 12:19

This was referenced Jan 23, 2024

Use AutoFile Sjors/bitcoin#32

Closed

CKey: add Serialize and Unserialize #29295

Draft

vasild reviewed Jan 23, 2024

View reviewed changes

willcl-ark added the Utils/log/libs label Jan 24, 2024

This was referenced Mar 13, 2024

scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint #29641

Draft

Several randomness improvements #29625

Open

achow101 closed this Apr 9, 2024

Make (Read/Write)BinaryFile work with char vector, use AutoFile #29229

Make (Read/Write)BinaryFile work with char vector, use AutoFile #29229

Conversation

Sjors commented Jan 11, 2024 • edited

DrahtBot commented Jan 11, 2024 • edited

Code Coverage

Reviews

Conflicts

Sjors commented Jan 11, 2024

Sjors commented Jan 11, 2024 • edited

DrahtBot commented Jan 11, 2024

Sjors commented Jan 12, 2024

Sjors commented Jan 12, 2024

vasild commented Jan 18, 2024

maflcko commented Jan 18, 2024

Sjors commented Jan 22, 2024 • edited

Sjors commented Jan 22, 2024

Sjors commented Jan 22, 2024

maflcko commented Jan 22, 2024

Sjors commented Jan 23, 2024

Sjors commented Jan 23, 2024

maflcko commented Jan 23, 2024

DrahtBot commented Jan 23, 2024

Sjors commented Jan 23, 2024 • edited

Sjors commented Jan 23, 2024 • edited

Choose a reason for hiding this comment

Sjors Jan 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Jan 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild Jan 24, 2024 • edited

Choose a reason for hiding this comment

Sjors Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors Jan 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Apr 9, 2024

Sjors commented Jan 11, 2024 •

edited

DrahtBot commented Jan 11, 2024 •

edited

Sjors commented Jan 11, 2024 •

edited

Sjors commented Jan 22, 2024 •

edited

Sjors commented Jan 23, 2024 •

edited

Sjors commented Jan 23, 2024 •

edited

Sjors Jan 23, 2024 •

edited

Sjors commented Jan 23, 2024 •

edited

vasild Jan 24, 2024 •

edited

Sjors Jan 25, 2024 •

edited

Sjors Jan 25, 2024 •

edited