Summary: | trustd network connections occasionally omitted from App Privacy report | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kate Cheney <katherine_cheney> | ||||||||||
Component: | WebKit Misc. | Assignee: | Kate Cheney <katherine_cheney> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, bfulgham, cdumez, cmarcelo, ddkilzer, ews-watchlist | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Kate Cheney
2021-11-05 15:24:00 PDT
Created attachment 443442 [details]
Patch
Comment on attachment 443442 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443442&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706 > + NSData *token = [NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]; nit: adoptNS([[NSData alloc] initWithBytes... > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:707 > + SecTrustSetClientAuditToken(challenge.nsURLAuthenticationChallenge().protectionSpace.serverTrust, (__bridge CFDataRef)token); SecuritySPI.h Created attachment 443510 [details]
Patch
Comment on attachment 443510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443510&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:704 > + if (auto auditToken = networkProcess().sourceApplicationAuditToken()) { It seems like all of this work is only needed if APP_PRIVACY_REPORT IS ENABLED, SO I’d widen the scope so you don’t create objects you don’t need for other build targets. You could use UNUSED_PARAM for the non-APR case. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706 > + RetainPtr<NSData> token = adoptNS([NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]); I think dataWithBytes returns an auto released value, so this adopt may cause an over release. Maybe check with Kilzer. Comment on attachment 443510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443510&action=review >> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706 >> + RetainPtr<NSData> token = adoptNS([NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]); > > I think dataWithBytes returns an auto released value, so this adopt may cause an over release. Maybe check with Kilzer. I also wonder if we could use dataWithBytesNoCopy to avoid copying the token (but perhaps the token is small enough to make the allocation a trivial cost) Comment on attachment 443510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443510&action=review >> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:704 >> + if (auto auditToken = networkProcess().sourceApplicationAuditToken()) { > > It seems like all of this work is only needed if APP_PRIVACY_REPORT IS ENABLED, SO I’d widen the scope so you don’t create objects you don’t need for other build targets. You could use UNUSED_PARAM for the non-APR case. +1 >>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706 >>> + RetainPtr<NSData> token = adoptNS([NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]); >> >> I think dataWithBytes returns an auto released value, so this adopt may cause an over release. Maybe check with Kilzer. > > I also wonder if we could use dataWithBytesNoCopy to avoid copying the token (but perhaps the token is small enough to make the allocation a trivial cost) Do not use adoptNS() here because as Brent noted it causes an over-release. Use adootNS([[NSData alloc] initWith…:]) to avoid creating an autoreleased object and to use adoptNS() correctly. Using a “NoCopy” variant means that the lifetime of the backing buffer must be the same as or longer than the NS object. Since the NSData object “escapes” via call to SecTrustSetClientAuditToken(), I would not recommend doing that. > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:708 > + SecTrustSetClientAuditToken(challenge.nsURLAuthenticationChallenge().protectionSpace.serverTrust, (__bridge CFDataRef)token.get()); You may use `bridge_cast(token.get())` here to magically do the cast to CFDataRef from NSData. Just #import <wtf/cocoa/TypeCastsCocoa.h> above. (In reply to David Kilzer (:ddkilzer) from comment #7) > Comment on attachment 443510 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443510&action=review > > >> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:704 > >> + if (auto auditToken = networkProcess().sourceApplicationAuditToken()) { > > > > It seems like all of this work is only needed if APP_PRIVACY_REPORT IS ENABLED, SO I’d widen the scope so you don’t create objects you don’t need for other build targets. You could use UNUSED_PARAM for the non-APR case. > > +1 > Will fix. > >>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:706 > >>> + RetainPtr<NSData> token = adoptNS([NSData dataWithBytes:(uint8_t *)&tokenValue length:sizeof(tokenValue)]); > >> > >> I think dataWithBytes returns an auto released value, so this adopt may cause an over release. Maybe check with Kilzer. > > > > I also wonder if we could use dataWithBytesNoCopy to avoid copying the token (but perhaps the token is small enough to make the allocation a trivial cost) > > Do not use adoptNS() here because as Brent noted it causes an over-release. > > Use adootNS([[NSData alloc] initWith…:]) to avoid creating an autoreleased > object and to use adoptNS() correctly. > Ah, I misread Alex's comment above and didn't add the [NSData alloc] part, only the adoptNS, I will fix. > Using a “NoCopy” variant means that the lifetime of the backing buffer must > be the same as or longer than the NS object. Since the NSData object > “escapes” via call to SecTrustSetClientAuditToken(), I would not recommend > doing that. > > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:708 > > + SecTrustSetClientAuditToken(challenge.nsURLAuthenticationChallenge().protectionSpace.serverTrust, (__bridge CFDataRef)token.get()); > > You may use `bridge_cast(token.get())` here to magically do the cast to > CFDataRef from NSData. > > Just #import <wtf/cocoa/TypeCastsCocoa.h> above. Nice! Will change. API test failures also look related :( Created attachment 443513 [details]
Patch
Comment on attachment 443513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443513&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:707 > + auto tokenValue = *auditToken; nit: I think auto& would save an unnecessary copy of a few bytes. Created attachment 443545 [details]
Patch for landing
(In reply to Alex Christensen from comment #11) > Comment on attachment 443513 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443513&action=review > > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:707 > > + auto tokenValue = *auditToken; > > nit: I think auto& would save an unnecessary copy of a few bytes. Thanks, fixed in PFL. Thanks Dave and Brent also for the review comments! Committed r285404 (243961@main): <https://commits.webkit.org/243961@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443545 [details]. |