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

[BUG] Darwin BleConnectionDelegateImpl::NewConnection deviceDiscriminator corrupted before dispatch async routine is called #26115

Closed
rob-the-dude opened this issue Apr 16, 2023 · 2 comments · Fixed by #26116

Comments

@rob-the-dude
Copy link
Contributor

I'm running the chip-tool on mac OS, and I noticed -- by putting a break point in the "dispatch_async" block of the "NewConnection" method (the one that takes a discriminator, not the new one that takes a connection object) -- that the device discriminator was "corrupted". I'm not as educated on how the C++ compiler handles stack-based objects for asynchronous blocks, but at a high level, this makes sense to me: we're passing in a C++ "const &" (reference) to the discriminator, which at the lowest level should be represented as some kind of pointer (and in this case, by looking at the caller, the discriminator object is on the stack itself), and by the time the dispatch queue runs, that stack location has new stuff in it.

As a simple fix, I changed the name of the passed in variable and then added a local variable that copies the passed in value and stores it for later. The async block stack magic seems to handle this case (if I understand the low level, by copying the needed value [as opposed to reference] onto the block's stack for later).

NOTE: I've been building the chip-tool using a self-created Xcode project, and it is POSSIBLE that there's some kind of compiler option that controls this behavior, which is set in the gn/ninja build and not in my project. I didn't see such a thing, but if it's there, then this issue can be ignored.

I'm providing a pull request that fixes this, but this really isn't my code at all, and I'd expect the Darwin team would want to fix this on their own.

rob-the-dude added a commit to rob-the-dude/connectedhomeip that referenced this issue Apr 16, 2023
The setup discriminator parameter is being passed by reference, but
by the time the block is run by dispatch, that reference points to
stack contents that have been changed.  Making a local copy of the
discriminator fixes the problem.
@rob-the-dude
Copy link
Contributor Author

I've created pull request #26116 to address this issue (it's still doing CI checks right now).

@bzbarsky-apple bzbarsky-apple linked a pull request Apr 17, 2023 that will close this issue
@bzbarsky-apple
Copy link
Contributor

@rob-the-dude Nice catch!

bzbarsky-apple added a commit that referenced this issue Apr 17, 2023
* Fix #26115: Setup discriminator parameter corruption

The setup discriminator parameter is being passed by reference, but
by the time the block is run by dispatch, that reference points to
stack contents that have been changed.  Making a local copy of the
discriminator fixes the problem.

* Add comment explaining why we are copying the incoming discriminator.

---------

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
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 a pull request may close this issue.

2 participants