Skip to content
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

_cbor_claim_bytes calculates wrong required for strings and bytestrings #106

Closed
woefulwabbit opened this issue Jul 31, 2019 · 2 comments · Fixed by #156
Closed

_cbor_claim_bytes calculates wrong required for strings and bytestrings #106

woefulwabbit opened this issue Jul 31, 2019 · 2 comments · Fixed by #156

Comments

@woefulwabbit
Copy link

woefulwabbit commented Jul 31, 2019

When _cbor_claim_bytes runs into a NEDATA condition, the result.required field is filled in with the size of the string data instead of the size of the header plus the string data. E.g. a 2193 bytestring returns a result.required of 2193 bytes instead of 2196 bytes.

Fix:

bool static _cbor_claim_bytes(size_t required, size_t provided,
                              struct cbor_decoder_result *result) {
  if (required > (provided - result->read)) {
    /* We need to keep all the metadata if parsing is to be resumed */
    result->status = CBOR_DECODER_NEDATA;
    result->required = result->read + required;
    result->read = 0;
    return false;
  } else {
    result->read += required;
    result->required = 0;
    return true;
  }
}
@PJK PJK added the bug label Jul 31, 2019
@PJK
Copy link
Owner

PJK commented Sep 4, 2020

Hi @woefulwabbit! I think that is working as intended, the documentation and naming might just be misleading.

cbor_decoder_result.required is the minimum number of additional bytes needed to continue decoding. You will see the same behavior even with integers, e.g. we would returnrequired == 1 for 0x18 even though we need 2B in total.

This is meant to make client-side buffering easier, but ultimately the relative and absolute conventions are arbitrary. You can just use source_size + cbor_decoder_result.required if you need the latter.

@PJK PJK added Documentation and removed bug labels Sep 4, 2020
@PJK
Copy link
Owner

PJK commented Sep 4, 2020

Sorry, I take that back. On second thought, the code actually doesn't implement what I described above. Counterexample: 0x19 returns required = 2, but 0x19 0x01 also returns required = 2. Two way to fix this:

  • Return the actual cbor_decoder_result.read instead of 0
  • Return the total number of bytes needed to proceed

I see how returning the total number of bytes needed to invoke the next callback given the current prefix would be more ergonomical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants