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

read: check that sizes are smaller than the file when reading #630

Merged
merged 1 commit into from
Feb 11, 2024

Conversation

symphorien
Copy link
Contributor

related to #204

added a regression test case which used to attempt to allocate 5 exabytes of memory.

test files added in gimli-rs/object-testfiles#12

tests/read/elf.rs Outdated Show resolved Hide resolved
@symphorien symphorien force-pushed the read_size_bound_check branch from ee5318d to 6ef1345 Compare February 10, 2024 17:31
Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think this is a good fix.

What's the motivation for the second test? It's fine to have it, but it appears to work prior to this fix.

Can you fix the CI failures and update the submodule to the merged commit.

@philipc
Copy link
Contributor

philipc commented Feb 11, 2024

Note that ReadCache is a bit of a hack. It's better to use memmap if you can. (This might be worth documenting.)

@symphorien symphorien force-pushed the read_size_bound_check branch from 6ef1345 to 04dde3d Compare February 11, 2024 10:32
@symphorien
Copy link
Contributor Author

Can you fix the CI failures and update the submodule to the merged commit.

done

What's the motivation for the second test? It's fine to have it, but it appears to work prior to this fix.

my initial implementation had an off by one error and get_buildid would always return Err() even on more legitimate binaries, the test is to ensure ReadCache can succeed.

@symphorien
Copy link
Contributor Author

Note that ReadCache is a bit of a hack. It's better to use memmap if you can. (This might be worth documenting.)

If that's the case yes please document it, and document preferred alternatives. My use case is to find the buildid of thousands of elf files in parallel. When I consulted the documentation of object, it seemed I needed to call File::parse so needed an implementation of ReadRef:

image

This leaves me with three options:

  • mmap: Most mmap rust wrappers come with huge unsafe warnings in the readme
  • reading the whole file in memory, which seemed wasteful at best
  • ReadCache, which is builtin to object

You can imagine that the third option seemed better at the time.

After using ReadCache in my code, I noticed memory usage was very spikey because of read cache so actually gave a try to a mmap implementation. I quickly abandoned the experiment, and if my memory serves me right here are the reasons:

  • it was slower
  • mmap would often (1 file out of ten) yield unexpected error codes like EINVAL (I don't remember exactly which)
  • spooky unsafe (please don't SIGBUS my process!)

I would be happy to discuss potential solutions or compare things more seriously and report the result if you are interested. (But maybe not on this PR)

@philipc philipc merged commit f1a0ec9 into gimli-rs:master Feb 11, 2024
11 of 12 checks passed
@philipc
Copy link
Contributor

philipc commented Feb 11, 2024

I default to using mmap and have never had problems with it. I sometimes use fs::read for small things that don't care about performance.

it was slower

I guess that might be true if you are only using this to read the build ID, where the total amount that you need to read is small.

mmap would often (1 file out of ten) yield unexpected error codes like EINVAL (I don't remember exactly which)

I haven't encountered that, and I don't know what could cause it. I'm interested in learning how to reproduce this.

spooky unsafe (please don't SIGBUS my process!)

Yes there's technically ways that this can be unsound, but mmap has a long history of being a useful tool. In isolation you'll never have a problem. The problem occurs when other code or other processes modify the file at the same time. I don't think that's a reason not to use it though, as long as you are aware of this limitation and evaluate how likely it is to occur for your use case. If you do know that the file will be modified, you'll have to take steps to prevent it happening at the same time you read, such as using locks.

@symphorien
Copy link
Contributor Author

I tried again and ReadCache is 47% faster for my use case. I found the source of the EINVAL: among all the files there were a quite large proportion of empty files, and mmaping empty files results in EINVAL.

@symphorien
Copy link
Contributor Author

Also about getting sigbus in case of concurrent modification of the file: I suppose that if you reimplement a cli tool the difference between sigbus and getting a real error is just the quality of the error message, but I'm implementing a server (debuginfod) and crashing the whole long lived process because of a single "wrong" file is quite annoying in this case. I suppose I could avoid that by forking and using the child process as a sacrificial victim just in case, but the increase in complexity in off-putting.

@symphorien symphorien deleted the read_size_bound_check branch March 3, 2024 12:32
@symphorien
Copy link
Contributor Author

would it be possible to get a new release with this change?

@philipc
Copy link
Contributor

philipc commented Mar 5, 2024

Published 0.33.0

mcbegamerxx954 pushed a commit to mcbegamerxx954/object that referenced this pull request Jun 15, 2024
…imli-rs#630)

This avoids large allocations due to invalid sizes in corrupted files.
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

Successfully merging this pull request may close these issues.

3 participants