-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Adding darwin callbacks for NOC Generation #21705
Adding darwin callbacks for NOC Generation #21705
Conversation
8af35db
to
5c16e52
Compare
PR #21705: Size comparison from c037fc2 to 5c16e52 Increases (24 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, p6)
Decreases (8 builds for cc13x2_26x2, esp32, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
5c16e52
to
a33bbc1
Compare
PR #21705: Size comparison from b1f2fc7 to a33bbc1 Increases (24 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6)
Decreases (8 builds for cc13x2_26x2, esp32, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
672ca85
to
08c750f
Compare
PR #21705: Size comparison from d502f30 to 08c750f Increases (26 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6)
Decreases (9 builds for cc13x2_26x2, esp32, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
08c750f
to
4fdd154
Compare
PR #21705: Size comparison from 2a5f7fe to 4fdd154 Increases (27 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6, telink)
Decreases (10 builds for cc13x2_26x2, esp32, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
4fdd154
to
b137409
Compare
PR #21705: Size comparison from 1f83af0 to b137409 Increases (25 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6)
Decreases (9 builds for cc13x2_26x2, esp32, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
b137409
to
79e5546
Compare
PR #21705: Size comparison from dfc1a70 to 45f112b Increases (30 builds for bl602, cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, psoc6, telink)
Decreases (9 builds for cc13x2_26x2, esp32, psoc6)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
* If ipk and adminSubject are passed, then they will be used in | ||
* the AddNOC command sent to the commissionee. If they are not passed, then the values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API says they are required to be non-nil, so must always be passed. Are they meant to be nullable?
__block chip::Optional<chip::Controller::CommissioningParameters> commissioningParameters; | ||
// Dereferencing mCppCommissioner as it would be set to point to a valid Cpp commissioner by now, as we are in the middle of | ||
// commissioning | ||
dispatch_sync(mChipWorkQueue, ^{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already running on the chip work queue? In which case, won't this deadlock?
certificationDeclaration:AsData(certificationDeclarationSpan) | ||
firmwareInfo:AsData(firmwareInfoSpan)]; | ||
|
||
dispatch_sync(mNocChainIssuerQueue, ^{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a sync dispatch?
// Dereferencing mCppCommissioner as it would be set to point to a valid Cpp commissioner by now, as we are in the middle of | ||
// commissioning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this true. We're coming here from some async callback from our API consumer. The commissioner could have been shut down by now, and then this is a use-after-free.
* Adding darwin callbacks for NOC Generation * Fixing compiler errors in new dependencies in src/credentials * Addressing feedback from woody-apple@ and bzbarsky-apple@ * Addressing second round of feedback * Address feedback around returning errors and adding comments * Adding comments Co-authored-by: Justin Wood <woody@apple.com>
Problem
Change overview
This change allows for custom credentials issuer implementations, for example, when a proprietary cloud API will perform the CSR signing.
See similar change made for android previously: #21522 and #21648