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

cmd: img_mgmt: Add an extra byte to the 'hash' buffer #5

Merged

Conversation

Olivier-ProGlove
Copy link
Contributor

_cbor_value_copy_string() (invoked by cbor_read_object()) adds a
null-character to the byte string.
Not adding this additional byte to the buffer obviously corrupts the memory.

Signed-off-by: Olivier Martin olivier.martin@proglove.de

cc: @carlescufi, @nvlsianpu

_cbor_value_copy_string() (invoked by cbor_read_object()) adds a
null-character to the byte string.
Not adding this additional byte to the buffer obviously corrupts the memory.

Signed-off-by: Olivier Martin <olivier.martin@proglove.de>
@carlescufi
Copy link
Contributor

@ccollins476ad can you please review? thanks!

@ccollins476ad
Copy link
Contributor

Thanks, Olivier. This looks like a good workaround for the buffer overrun.

The real bug seems to be in the TinyCbor implementation. From _cbor_value_copy_string's heading:

 * On success, this function sets the number of bytes copied to \c{*buflen}. If
 * the buffer is large enough, this function will insert a null byte after the
 * last copied byte, to facilitate manipulation of null-terminated strings.
 * That byte is not included in the returned value of \c{*buflen}.

However, this particular bug is not in the official TinyCbor source (https://github.com/intel/tinycbor), and it is also not in the Mynewt fork. I guess this bug must be an artifact from the original merge to Zephyr.

@carlescufi
Copy link
Contributor

@ccollins476ad I don’t have merge rights on this repo. Mind merging this yourself? Thanks!

@carlescufi
Copy link
Contributor

Cc @MariuszSkamra @sjanc

@ccollins476ad ccollins476ad merged commit c2da8ca into apache:master Jul 2, 2018
@carlescufi
Copy link
Contributor

@Olivier-ProGlove @JoeHut will you send a PR to Zephyr to update with this version?

@JoeHut
Copy link
Contributor

JoeHut commented Jul 3, 2018

@carlescufi Sure.

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

4 participants