Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
PE: parse thread local storage - TLS data #404
PE: parse thread local storage - TLS data #404
Changes from 1 commit
dbe61df
b76da8b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a malformed binary could this cause an infinite loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think it would make sense to have a RVA validation there and return the malformed error if invalid RVAs found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what i meant specifically is: is this loop guaranteed to terminate? this comes up when there are malformed binaries, and the loop only terminates in a well-formed case, but if the loop has no upper bound, it might cause the library to run forever, which is unacceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RVA check I previously introduced should be sufficient to address the malformations. In general, it does not result in an infinite loop. However, there might be instances where it could raise slice bound errors. Typically, the TLS callback array is located within the data sections—a common characteristic across multiple PECOFF compilers—and the data section is well-aligned with zeros. In my experience handling over 10,000 to 20,000 unique PECOFF binaries, I have not encountered any infinite loops with this implementation. Nonetheless, I believe setting a reasonable limit would be prudent. Is this the solution you were seeking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I was just making sure there wasn't some obvious termination condition we could add, e.g., end of file, etc., that would guarantee we are done looping; my general reflex when i see
loop
is to ask how it can terminate, as we've had bugs in past of infinite loops without proper termination condition, but your answer is sufficient, thank you!