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 buffer overflow error in cel3ds, improve error handling #1145

Merged
merged 5 commits into from
Nov 5, 2021
Merged

Fix buffer overflow error in cel3ds, improve error handling #1145

merged 5 commits into from
Nov 5, 2021

Conversation

ajtribick
Copy link
Collaborator

I noticed a potential buffer overflow error in the readString function in 3dsread.cpp: if the string is 1024 characters or longer, the terminating zero byte is not written and the string constructor from const char* will therefore read beyond the end of the buffer.

The rest of the loader functionality was in my opinion far too trusting that the input files are not malformed. I've therefore added various error checking that was missing, including verifying that the stream is in a good state after attempting to read data, and consistency checks that the expected amount of data has been read. I also replaced the void pointer usage with templates to ensure better type safety, and switched to using std::make_unique rather than new to simplify cleanup on the error path.

So far I've checked that the Apollo 11 and Mir models still load.

@ajtribick
Copy link
Collaborator Author

ajtribick commented Oct 29, 2021

The code analysis report for the comma operator in matrix initialization is a known issue: https://trac.cppcheck.net/ticket/10518

As for the "Check buffer boundaries if used in a loop including recursive loops" - I genuinely don't know what it wants me to do here. The call to read is using the size of the buffer defined immediately beforehand. I think this is the same issue as david-a-wheeler/flawfinder#59

src/cel3ds/3dsread.cpp Outdated Show resolved Hide resolved
src/cel3ds/3dsread.cpp Show resolved Hide resolved
Copy link
Collaborator

@375gnu 375gnu left a comment

Choose a reason for hiding this comment

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

sometimes c++ is too stupid. but let's roll

@375gnu 375gnu requested a review from levinli303 November 3, 2021 14:47
@375gnu
Copy link
Collaborator

375gnu commented Nov 4, 2021

@ajtribick we usually merge own PRs ourselves so feel free to merge this if you consider it ready.

@ajtribick ajtribick merged commit e00f5b7 into CelestiaProject:master Nov 5, 2021
@ajtribick ajtribick deleted the cel3dsfix branch November 5, 2021 18:53
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