Repository navigation
flexbuffers: bound verification work and fix depth leak on shared vectors - #9442
Open
17krishna8 wants to merge 1 commit into
Open
17krishna8 wants to merge 1 commit into
17krishna8 wants to merge 1 commit into
Conversation
…tors VerifyVector() charged one unit of work per vector visit, but not for the elements visited inside each vector. A vector can be shared by any number of references, and when no reuse tracker is used, each visit of a shared vector walks all of its elements again, so the total amount of verification work is only bounded by the product of the number of references and the largest vector size. A hand-crafted ~200KB buffer that is valid and verifies successfully can keep a CPU busy for a long time in the default configuration (measured: 185ms for 200KB, growing linearly with the number of references). Charge each visited element to the work counter that is capped by the buffer size, so that such a buffer is rejected quickly instead. Buffers that don't share vectors are unaffected; heavily shared buffers should be verified with a reuse tracker, which is now also documented. The reuse tracker's memoized early return in VerifyVector() also returned without releasing the depth acquired on entry, so a buffer sharing a vector more than max_depth (64) times failed verification when a tracker was used, even though the buffer was valid. Release the depth on that path. Add FlexBuffersVerifierWorkBoundTest(): a crafted buffer sharing a 65535-element vector 1000 times is rejected quickly without a tracker, accepted with one, and normal buffers still verify without a tracker.
This branch has not been deployed
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
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.
flexbuffers::VerifyVector()charges one unit of work (num_vectors_, capped at the buffer length) per vector visit, but nothing for the elements visited inside each vector. A vector can be shared by any number of references, and when no reuse tracker is used, every visit of the shared vector walks all of its elements again, so the total amount of verification work is only bounded by the product of the number of references and the largest vector size.Concretely, a hand-crafted buffer consisting of one heterogeneous vector of 65535 inline ints, shared by 1000 references:
flexbuffers::VerifyBuffer(buf, len)with no reuse tracker), growing linearly with the number of references, so a few MB are enough for minutes of CPU time on one small input.This change charges each visited element to the same work counter, which is capped at the buffer length, so such a buffer is now rejected quickly instead:
trueafter 185ms (measured), unbounded growth,falsein 0ms.Buffers that do not share vectors are unaffected: the elements of every vector occupy at least one byte each (two for heterogeneous vectors: one offset/type byte each), so the total charge for a buffer without sharing is always below the buffer length. Buffers with heavy sharing should be verified with a reuse tracker; with a tracker, each distinct vector's elements are walked once, so those buffers stay within the bound as well. The constructor comment now documents this.
While testing the tracker path, I also found that the reuse tracker's memoized early return in
VerifyVector()returns without releasing the depth acquired on entry, so any buffer sharing a vector more thanmax_depth(64) times fails verification when a tracker is used, even though the buffer is valid. The second hunk releases the depth on that path; the same crafted buffer now verifies successfully with a tracker.Tests:
FlexBuffersVerifierWorkBoundTest()builds the crafted buffer described above by hand and checks that it is rejected without a tracker, accepted with one, and that a normal non-sharing builder-generated buffer still verifies without a tracker.FlexBuffersTest(),FlexBuffersReuseBugTest()andFlexBuffersDeprecatedTest()pass unchanged.