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

Fails gracefully when allocation fails during array/map construction #224

Merged
merged 3 commits into from
Dec 3, 2022

Conversation

James-ZHANG
Copy link
Contributor

@James-ZHANG James-ZHANG commented Sep 29, 2022

Fails gracefully when allocation fails during array/map construction

Description

Return values of cbor_array_push, _cbor_map_add_key and _cbor_map_add_value are unchecked. An unchecked false return value here implies potential out-of-bound memory access.

Checklist

  • I have read followed CONTRIBUTING.md
    • I have added tests
    • I have updated the documentation
    • I have updated the CHANGELOG
  • Are there any breaking changes? If so, are they documented?
    No
  • Does this PR introduce any platform specific code? If so, is this captured in the description?
    No
  • Security: Does this PR potentially affect security? If so, is this captured in the description?
    No
  • Performance: Does this PR potentially affect performance? If so, is this captured in the description?
    No

@PJK
Copy link
Owner

PJK commented Oct 18, 2022

Apologies for the delay and thanks a ton for the PR!

Isn't there still a problem with the memory cleanup? I think that when a nested _cbor_builder_append fails, all the partially built items on the ctx->stack should be freed up, but creation_failed is never checked as we "unwind the stack"

@James-ZHANG
Copy link
Contributor Author

all the partially built items on the ctx->stack should be freed up

Those are already freed up in cbor_load.

I think the invariant of _cbor_builder_append is it shouldn't have any memory leak: the passed item either becomes part of stack->root; or in case of failure, gets freed properly (and in that case it leaves partially built stack items behind).

Added a function comment emphasizing that.

@PJK
Copy link
Owner

PJK commented Dec 3, 2022

Makes sense, thank you! This could use better docs, filed #226

@PJK PJK merged commit ff66e8c into PJK:master Dec 3, 2022
PJK added a commit that referenced this pull request Dec 3, 2022
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.

None yet

2 participants