Bug 232770 – trustd network connections occasionally omitted from App Privacy report

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

trustd network connections occasionally omitted from App Privacy report

[Kate Cheney]

rdar://83840427

[Kate Cheney]

Created attachment 443442 [details]
Patch

[Alex Christensen]

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

[Kate Cheney]

Created attachment 443510 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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)

[David Kilzer (:ddkilzer)]

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.

[Kate Cheney]

(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.

[Kate Cheney]

API test failures also look related :(

[Kate Cheney]

Created attachment 443513 [details]
Patch

[Alex Christensen]

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.

[Kate Cheney]

Created attachment 443545 [details]
Patch for landing

[Kate Cheney]

(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!

[EWS]

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].