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

Fix large heap allocation during load broken files. #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Vol-Alex
Copy link

No description provided.

Comment on lines 390 to 391
else if (len > size())
{

Choose a reason for hiding this comment

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

Probably it should be else if (....) {.

kaitai/kaitaistream.cpp Outdated Show resolved Hide resolved
kaitai/kaitaistream.cpp Outdated Show resolved Hide resolved
} else if (len == 0) {
return std::string();
} else if (len > size()) {
throw std::runtime_error("read_bytes: requested length greater than stream size");
}
Copy link
Member

Choose a reason for hiding this comment

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

In general, that's a bad idea to call size() if we can avoid that. First of all, not all streams are finite and have sizes at all. The way that size() is implemented in KS runtime is by seeking to end of stream, getting position index there, and going back to pre-memorized original location. It won't work on infinite streams like ones coming from IO port or networking socket.

Moreover, reading past end-of-stream throws a well-defined exception like std::ifstream::failure, is (1) arguably better than throwing a std::runtime_error with some text, (2) relied upon in existing codebases.

Copy link
Author

Choose a reason for hiding this comment

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

As I understand kaitaia::ostream can not work with infinite/non seekable streams. See the realization of kaitai::kstream::read_bytes_term(...), kaitai::kstream::size(...), kaitai::kstream::seek(...).

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.

4 participants