Skip to content

flexbuffers: bound verification work and fix depth leak on shared vectors - #9442

Open
17krishna8 wants to merge 1 commit into
google:masterfrom
17krishna8:verifier-bound-element-visits
Open

17krishna8 wants to merge 1 commit into
google:masterfrom
17krishna8:verifier-bound-element-visits

Conversation

@17krishna8

Copy link
Copy Markdown

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:

  • is ~200KB in size,
  • is a valid flexbuffer, and verifies successfully,
  • takes 185ms to verify in the default configuration (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:

  • before: ~200KB crafted buffer → true after 185ms (measured), unbounded growth,
  • after: the same buffer → false in 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 than max_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() and FlexBuffersDeprecatedTest() pass unchanged.

…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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant