Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't read content > 4GB with Content Receiver on 32bit platform #2084

Open
GEEKiDoS opened this issue Mar 1, 2025 · 4 comments
Open

Can't read content > 4GB with Content Receiver on 32bit platform #2084

GEEKiDoS opened this issue Mar 1, 2025 · 4 comments

Comments

@GEEKiDoS
Copy link

GEEKiDoS commented Mar 1, 2025

I'm using the client

@GEEKiDoS GEEKiDoS changed the title Can't read content > 4GB even with Content Receiver in 32bit platform Can't read content > 4GB with Content Receiver on 32bit platform Mar 1, 2025
@GEEKiDoS
Copy link
Author

GEEKiDoS commented Mar 1, 2025

seems like the size_t issue, modified some of variable types to uint64_t works fine

  • read_len in read_content_with_length
  • payload_max_length on read_content
  • std::numeric_limits<size_t> to std::numeric_limits<uint64_t> in detail::read_content call

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Mar 3, 2025

Some other read functions already use uint64_t, FYI. Seems like an oversight.

Edit: Looking more closely, I'm unsure how to square storing the body in std::string, which imposes a very real limit on 32-bit systems, with the content receiver API, which is not necessarily subject to the same constraints. IMO, it would make sense for the content receiver API to bypass max payload length. Also, max payload length should be set (and clamped) to std::string::max_size() – not std::numeric_limits<size_t>::max() – as those values differ on all implementations I'm familiar with.
Alternatively, max payload length needs to be changed to uint64_t, and when the destination of the body is a std::string, we clamp it to std::string::max_size(). Maybe that's the least surprising choice.

@yhirose
Copy link
Owner

yhirose commented Mar 5, 2025

@GEEKiDoS @falbrechtskirchinger thanks for the report and investigation!

@GEEKiDoS Since I don't have a 32bit machine, I am not able to reproduce and confirm this issue at this point. Could you make a Unit test case in test/test.cc which can reproduce this issue, and send me a pull request including the test case? Meanwhile, I'll try to find an easy way to build a 32bit executable and run it on 64 bit Ubuntu on my 64 bit CPU machine. Thanks a lot!

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Mar 6, 2025

Meanwhile, I'll try to find an easy way to build a 32bit executable and run it on 64 bit Ubuntu on my 64 bit CPU machine. Thanks a lot!

That's as easy as adding -m32. I have some experience with this 32-bit and size_t issue from nlohmann/json. I implemented the 32-bit unit tests in nlohmann/json#3523.

I did start work on a PR but quickly gave up after realizing what other problems this issue reveals:

  • Only one compressor can handle >4GB on 32-bit without modification.
  • File handling is severely limited without memory-mapped files (1GB on some implementations). Memory-mapped files are capped by the address space, which can be worked around with offsets. (Edit: read_file() is only used by test.cc)
  • Some places check std::string::max_size() but not all. None set the proper status code.

It's more realistic to document the limitations of 32-bit systems first and then address some of the low-hanging fruit. Triggering all issues in unit tests may not be possible due to RAM limits.

If I find the time later, I'll work on documentation and some Makefile changes to allow local testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants