Limit binary fragment size during applies#77
Merged
Conversation
Applying a binary fragment can use a lot of memory in two ways: 1. Inflating a large amount of literal data 2. Processing a delta instruction stream with many copy operations Clients could theoretically protect against the first by checking fragment sizes for all binary fragments before the apply, but there's no way to protect against the second case except by decoding the delta instruction stream. And because the first case used to happen at parse time, there are no existing clients that check this before applying. Fix both issues by enforcing a maximum binary fragment size. The default value is 100MiB, which matches GitHub.com's default maximum file size. Clients can adjust or disabled this limit by passing new option functions to Apply, NewTextApplier, and NewBinaryApplier. Finally, fix an encoding issue in the bin.go helper utility by using the standard BinaryFragment string representation instead of implementing the encoding directly with bad wrapping logic. This was required to generate the test patches for applies over the limit. The `.hex` files are not used directly by the test, but provide the source input for the patches as processed by `bin.go`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Applying a binary fragment can use a lot of memory in two ways:
Clients could theoretically protect against the first by checking fragment sizes for all binary fragments before the apply, but there's no way to protect against the second case except by decoding the delta instruction stream. And because the first case used to happen at parse time, there are no existing clients that check this before applying.
Fix both issues by enforcing a maximum binary fragment size. The default value is 100MiB, which matches GitHub.com's default maximum file size. Clients can adjust or disabled this limit by passing new option functions to Apply, NewTextApplier, and NewBinaryApplier.
Finally, fix an encoding issue in the bin.go helper utility by using the standard BinaryFragment string representation instead of implementing the encoding directly with bad wrapping logic. This was required to generate the test patches for applies over the limit. The
.hexfiles are not used directly by the test, but provide the source input for the patches as processed bybin.go.