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

cb: Fix uninitialized memory used for equality check #5027

Merged
merged 1 commit into from Sep 27, 2017

Conversation

Projects
None yet
7 participants
@geky
Member

geky commented Sep 5, 2017

Fixed by zeroing the uninitialized memory. A more "correct" approach may be to add an additional op for equality, and use the "==" operator on the F type in the generate function. But this adds the requirement that F supports equality.

cc @0xc0170, @pan-
fixes #5017

cb: Fixed uninitialized memory used for equality check
Fixed by zeroing the memory. A more "c++ correct" approach
may be to add an additional op for equality, and use the "=="
operator on the F type in the generate function. But this adds
the requirement that F supports equality.

@geky geky requested review from pan- and 0xc0170 Sep 5, 2017

@geky geky added the needs: review label Sep 5, 2017

@geky geky referenced this pull request Sep 5, 2017

Closed

Callback: comparision fails #5017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 6, 2017

My first question is: Is callback class POD? I can see it has user-defined dtor at least. If it is, then memxxx can be used, otherwise it should not (neither to use memcmp as I pinpointed earlier in the == operator).

@bulislaw

This comment has been minimized.

Member

bulislaw commented Sep 6, 2017

I've created internal ticket to write some more tests for callback.

@geky

This comment has been minimized.

Member

geky commented Sep 6, 2017

@0xc0170 The POD restriction on memxxx is for external use. The Callback is aware of its own operations and makes sure not to break their assumptions.

The Callback is already handling it's own memory explicitly:
https://github.com/ARMmbed/mbed-os/blob/master/platform/Callback.h#L516-L517

@bulislaw agreed, this was just a quick patch so I didn't have time but good idea to track

@pan-

pan- approved these changes Sep 6, 2017

That's the best fix for now however it might be a good thing to precise that comparison operation won't work if the callback class host a Function like object which is not a standard layout type.

Providing a complete, correct solution for all case is a not a trivial task. Indeed, there is no operator== to compare an std::function to another std::function in the standard library.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 7, 2017

@0xc0170

0xc0170 approved these changes Sep 7, 2017

@geky

This comment has been minimized.

Member

geky commented Sep 21, 2017

@0xc0170, is this guy viable for 5.6.1?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

Probably can't make it, but it could if testing comes back fast enough.

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 22, 2017

Result: ABORTED

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1365

Build failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 23, 2017

/morph test

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 27, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 27, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1401

All builds and test passed!

@theotherjimmy theotherjimmy merged commit 5bf224f into ARMmbed:master Sep 27, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment