Fix the dnssd code that browses on both the local and srp domains #32675
Conversation
- Fixes the UAF issue with the timer - Resolves critical comments from PR project-chip#32631
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple and
lazarkov
pullapprove
bot
requested review from
LuDuda,
mhazley,
mkardous-silabs,
mspang,
p0fi,
pidarped,
plauric,
rcasallas-silabs,
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #32675: Size comparison from 1f2aff4 to d5ca8db Full report (41 builds for cc13x4_26x4, cc32xx, cyw30739, k32w, nrfconnect, psoc6, qpg, stm32, telink)
|
| protocol = GetProtocol(cbAddressType); | ||
| instanceName = instanceNameToResolve; | ||
| consumerCounter = std::move(consumerCounterToUse); | ||
| hasSrpTimerStarted = false; |
There was a problem hiding this comment.
Why is this not default-initialized in the class decl, so you don't have to do it in both constructors?
I know other members were doing things this way; that's not a reason to copy the bad pattern.
There was a problem hiding this comment.
I fixed this after Karsten's comment. i already have it defaulting to false in the class decl.
There was a problem hiding this comment.
i already have it defaulting to false in the class decl.
Then we shouldn't touch it in the constructor.
| auto interfaceId = interface.first.first; | ||
| if (TryReportingResultsForInterfaceIndex(interfaceId)) |
There was a problem hiding this comment.
This will "over-report" if we have multiple domain responses for the same interface, right? That seems pretty undesirable..... We really do want a TryReportingResultsForInterfaceIndex that takes both args, and then the single-arg version needs to make multiple calls to it for all the matching entries or something.
But before you start changing this around, we need to figure out what actually needs to be stored as part of keys here, and what needs to be stored as part of values.
There was a problem hiding this comment.
The reason I couldn't add the domain here is because there are hardcoded interfaces to prioritize that do not have a domain and I have no idea what domains to use here. we need to discuss this more for me to fix this properly.
There was a problem hiding this comment.
My point is that the "prioritize" bits indeed can only pass in the interface id (and then walk through all the matches and call the two-arg version for each one), but the bit here should pass in both pieces.
| // Some interface may not have any ips, just ignore them. | ||
| if (ips.size() == 0) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
Shouldn't this be a continue? Returning here looks wrong to me.
There was a problem hiding this comment.
I didn't modify this code. :) but yes I see we need to continue to the other interfaces
There was a problem hiding this comment.
I didn't modify this code.
You changed the context around it, though. Before, when there was only map entry the code was dealing with, return was correct. Now that it's trying to work with multiple map entries, returning is not correct.
So not modifying this code is the bug.
There was a problem hiding this comment.
yup. got that after i closley inspected the changes. it is a bug and i fixed it locally.
| */ | ||
| void CancelSrpTimer(ResolveContext * ctx) | ||
| { | ||
| DeviceLayer::SystemLayer().CancelTimer(SrpTimerExpiredCallback, reinterpret_cast<void *>(ctx)); |
There was a problem hiding this comment.
Why do you need the cast here? Just passing ctx as the second arg should work. Same in StartTimer....
| @@ -307,25 +309,37 @@ static void OnGetAddrInfo(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t i | |||
| { | |||
| VerifyOrReturn(sdCtx->HasAddress(), sdCtx->Finalize(kDNSServiceErr_BadState)); | |||
|
|
|||
| if (domainName.compare(kOpenThreadDot) == 0) | |||
| if (domainName.compare(kSrpDot) == 0) | |||
There was a problem hiding this comment.
I think this is broken. You can get multiple SRP results, then a non-SRP result with !kDNSServiceFlagsMoreComing, and this code will end up sitting and waiting for 250ms when it shouldn't. I think there's some confusion here about what kDNSServiceFlagsMoreComing means. Which is understandable, because it can be confusing. If set, all it means is "something on this same connection has more data to deliver right now". In our case, it could be either of the two resolves, any of the GetAddrInfo those kicked off, etc. In particular, it does not mean "no more data coming for the args that got passed here"; it means "no more stuff I know about right this second for anything that is using this connection".
| ChipLogProgress(Discovery, "Mdns: Resolve completed on the open thread domain."); | ||
| ChipLogProgress(Discovery, "Mdns: Resolve completed on the srp domain."); | ||
|
|
||
| // Cancel the timer if one has been started |
There was a problem hiding this comment.
Why bother? The Finalize will destroy the context which cancels the timer, no?
| // Schedule a timer to allow the resolve on Srp domain to complete. | ||
| CHIP_ERROR error = StartSrpTimer(kSrpTimeoutInMsec, sdCtx); | ||
|
|
||
| // If the timer fails to start, finalize the context and return. |
There was a problem hiding this comment.
This comment should explain why, not what. The "what" (finalize and return) is obvious from the code below. The question is: why is that the right thing to do? (It is, but the comment should explain why.)
Fixes the UAF issue with the timer
Resolves critical comments from PR Dnssd changes to browse and resolve using open thread domain along wi… #32631
Fixes: #32668