Skip to content

THRIFT-5383 Fix misplaced capacity check#2366

Closed
aaronstgeorge-wf wants to merge 2 commits intoapache:masterfrom
aaronstgeorge-wf:THRIFT-5383_buffer_bounds_check
Closed

THRIFT-5383 Fix misplaced capacity check#2366
aaronstgeorge-wf wants to merge 2 commits intoapache:masterfrom
aaronstgeorge-wf:THRIFT-5383_buffer_bounds_check

Conversation

@aaronstgeorge-wf
Copy link
Copy Markdown
Contributor

@aaronstgeorge-wf aaronstgeorge-wf commented Mar 29, 2021

The added test passes with thrift 0.13.0 and fails on 0.14.0. The source of the error seems to be an unnecessary capacity check when reading JSON string.

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@aaronstgeorge-wf aaronstgeorge-wf changed the title THRIFT-5383 Remove unnecessary bounds check THRIFT-5383 Remove unnecessary capacity check Mar 30, 2021
@Jens-G Jens-G changed the title THRIFT-5383 Remove unnecessary capacity check THRIFT-5383 Fix misplaced capacity check Mar 30, 2021
@aaronstgeorge-wf
Copy link
Copy Markdown
Contributor Author

aaronstgeorge-wf commented Mar 30, 2021

I'm not sure how we could know the size before reading the string? Because this is in the JSON protocol there is no preceding size packed in the data. All you have is the open quote character and you have to read to the end. This check makes sense for compact, binary, et al because they are serialized with the size of the string contained after, but the serialized JSON representation has no such thing. It's just a quote-delimited string.

@aaronstgeorge-wf aaronstgeorge-wf force-pushed the THRIFT-5383_buffer_bounds_check branch from 96b703a to 65be602 Compare March 30, 2021 17:37
@aaronstgeorge-wf
Copy link
Copy Markdown
Contributor Author

aaronstgeorge-wf commented Mar 30, 2021

I think we we got off on the wrong foot here. Where should that line be? I'm quite happy to move it and write a test that asserts it does what it is supposed to do. From what I know about the JSON protocol, I'm just not sure what it's supposed to do, or where it's supposed to be. I am not a thrift expert.

@Jens-G Jens-G closed this in fa87d0e Mar 30, 2021
Jens-G pushed a commit to Jens-G/thrift that referenced this pull request Jun 10, 2021
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.

1 participant