util: Fix `ReadBinaryFile` reading beyond maxsize #24371

Currently ReadBinaryFile will read beyond maxsize if maxsize is not a multiple of 128 (size of buffer)

This is due to fread being called with count = 128 instead of count = min(128, maxsize - retval.size() at every iteration

The following unit test will fail:

BOOST_AUTO_TEST_CASE(util_ReadWriteFile)
{
  fs::path tmpfolder = m_args.GetDataDirBase();
  fs::path tmpfile = tmpfolder / "read_binary.dat";
  std::string expected_text(300,'c');
  {
      std::ofstream file{tmpfile};
      file << expected_text;
  }
  {
      // read half the contents in file
      auto [valid, text] = ReadBinaryFile(tmpfile, expected_text.size() / 2);
      BOOST_CHECK_EQUAL(text.size(), 150);
  }
}

Error:

test/util_tests.cpp:2593: error: in "util_tests/util_ReadWriteFile": check text.size() == 150 has failed [256 != 150]

Looks like your patch causes a "global buffer overflow" in the ASan CI task

Concept ACK.

Is there a reason we don't read directly into the std::string?

Is there a reason we don't read directly into the std::string? @luke-jr If I am not wrong, it is not advisable to fread into std::string as c_str() is const and we will also need to know the size of the file beforehand to allocate the right number of characters. More Details

Looks like your patch causes a "global buffer overflow" in the ASan CI task @MarcoFalke fixed the ASan issue in the unit test.

If I am not wrong, it is not advisable to fread into std::string as c_str() is const

I would expect to use .begin(), not .c_str()

and we will also need to know the size of the file beforehand to allocate the right number of characters.

I would be surprised if repeatedly calling .resize was slower than repeatedly calling .append.

Concept ACK

I would expect to use .begin(), not .c_str() @luke-jr I am not sure how to do that. From what I understand fread takes in void * but .begin() returns an iterator. Do you have an example of how I can change it to use .begin()?

Code-review ACK a84650ebd5ac2cbb49f14eb7c98736a3f8215bf1

@luke-jr I am not sure how to do that.

Let's consider it out of scope for this PR and stick to a simpler fix. :)

Code review ACK a84650ebd5ac2cbb49f14eb7c98736a3f8215bf1

@luke-jr If I am not wrong, it is not advisable to fread into std::string as c_str() is const and we will also need to know the size of the file beforehand to allocate the right number of characters.

I would really discourage such hacks if not absolutely required. ReadBinaryFile is not used in any performance critical contexts, and it's only used to read like 64 bytes maximum, so please go with a simple and straightforward solution even if not the fastest.

Edit: and thinking of it, conceptually std::string is the wrong type for a binary blob. Something like std::vector<byte> would be better. In any case, out of scope.

utACK