Bug 210304

<rdar://problem/61155623>

Created attachment 396015 [details]
Patch

Created attachment 396016 [details]
Patch

Created attachment 396019 [details]
Patch

Comment on attachment 396019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396019&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:424
> +    std::strncpy(cachedHeader.osVersion, osVersion.utf8().data(), versionLength);

Would be slightly nicer to use sizeof(cachedSandboxHeader.osVersion) here, or to use versionLength below. That way, the length we copy in the equal to the length we compare.

Relatedly: What guarantees that systemMarketingVersion() will always return a string that is equal in size to versionLength? Seems like we need a length check before the copy. And maybe we should reserve extra space in case systemMarketingVersion() starts returning something bigger in the future?

Comment on attachment 396019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396019&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:469
>      if (cachedSandboxHeader.versionNumber != CachedSandboxVersionNumber)

Style wise it really feels like this should be the first branch.

Comment on attachment 396019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396019&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:469
>      if (cachedSandboxHeader.versionNumber != CachedSandboxVersionNumber)

Style wise it really feels like this should be the first branch.

Comment on attachment 396019 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396019&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:94
> +constexpr unsigned guidLength = 36 + 1;

Nit: I’d name these “size” since length is really this - 1

Created attachment 396356 [details]
Patch

Comment on attachment 396356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396356&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:427
> +    std::strncpy(cachedHeader.sandboxBuildID, SANDBOX_BUILD_ID, sizeof(cachedHeader.sandboxBuildID));
> +    RELEASE_ASSERT(!cachedHeader.sandboxBuildID[guidSize - 1]);
> +    std::strncpy(cachedHeader.osVersion, osVersion.utf8().data(), sizeof(cachedHeader.osVersion));
> +    RELEASE_ASSERT(!cachedHeader.osVersion[versionSize - 1]);

It’s almost never good to use strncpy; I suggest using strlcpy instead.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:476
> +    if (std::strncmp(cachedSandboxHeader.sandboxBuildID, SANDBOX_BUILD_ID, sizeof(cachedSandboxHeader.sandboxBuildID)))
>          return false;
> +    if (std::strncmp(cachedSandboxHeader.osVersion, osVersion.utf8().data(), sizeof(cachedSandboxHeader.osVersion)))
> +        return false;

These should be strcmp; no reason I can think of to use strncmp to compare two strings that are both null-terminated.

Comment on attachment 396356 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396356&action=review

> Source/WebKit/ChangeLog:10
> +        We recently discovered that the system sandbox framework version does not always change
> +        when breaking changes in the sandbox format are made. This can lead to the precompiled

Logically, this feels very weird. Can we tell them not to do that?

I'm not against us adding the code you're adding here, too. It just feels weird that they can make a breaking change without updating the version.

There are many system dylibs that are not set up to ever update the version, as there is very little observable effect, if any. Still, see rdar://problem/61155623.

Created attachment 396448 [details]
Patch

Comment on attachment 396448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396448&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:425
> +    ASSERT(copied = guidSize - 1);

==

(In reply to Geoffrey Garen from comment #14)
> Comment on attachment 396448 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396448&action=review
> 
> > Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:425
> > +    ASSERT(copied = guidSize - 1);
> 
> ==

Oh hell. Fixing.

Created attachment 396453 [details]
Patch

Comment on attachment 396453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396453&action=review

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:426
> +    RELEASE_ASSERT(!cachedHeader.sandboxBuildID[guidSize - 1]);

OK to have this, but overkill since we are using strlcpy, which guarantees null termination. That’s it’s whole reason for existing.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:428
> +    RELEASE_ASSERT(!cachedHeader.osVersion[versionSize - 1]);

Ditto.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:477
> +    if (std::strcmp(cachedSandboxHeader.osVersion, osVersion.utf8().data()))

Given that this is an ASCII string, this can just be written:

    if (cachedSandboxHeader.osVersion != systemMarketingVersion())

The String class already has a != operator that works.

Comment on attachment 396453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396453&action=review

>> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:426
>> +    RELEASE_ASSERT(!cachedHeader.sandboxBuildID[guidSize - 1]);
> 
> OK to have this, but overkill since we are using strlcpy, which guarantees null termination. That’s it’s whole reason for existing.

I'll remove both of these.

>> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:477
>> +    if (std::strcmp(cachedSandboxHeader.osVersion, osVersion.utf8().data()))
> 
> Given that this is an ASCII string, this can just be written:
> 
>     if (cachedSandboxHeader.osVersion != systemMarketingVersion())
> 
> The String class already has a != operator that works.

Okay -- that's much tidier.

Created attachment 396459 [details]
Patch for landing

Committed r260098: <https://trac.webkit.org/changeset/260098>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396459 [details].

This is actually <rdar://problem/61537700>.