[webkit-reviews] review granted: [Bug 210616] [iOS] Deny iokit open access to graphics related classes : [Attachment 396707] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 210616: [iOS] Deny iokit open access to graphics related classes
https://bugs.webkit.org/show_bug.cgi?id=210616

Attachment 396707: Patch

https://bugs.webkit.org/attachment.cgi?id=396707&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 396707
  --> https://bugs.webkit.org/attachment.cgi?id=396707
Patch

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

> Source/WebCore/platform/cocoa/AGXCompilerService.cpp:48
> +	   if (uname(&systemInfo))
> +	       return false;

Is this the behavior we want, to return a potentially incorrect "false" result
and to also repeatedly call uname every time this function is called?

> Source/WebCore/platform/cocoa/AGXCompilerService.cpp:53
> +	   if (!strcmp(machine, "iPad5,1") || !strcmp(machine, "iPad5,2") ||
!strcmp(machine, "iPad5,3") || !strcmp(machine, "iPad5,4"))
> +	       hasAGXCompilerService = true;
> +	   else
> +	       hasAGXCompilerService = false;

Apparently this code was just moved, not new, but we want this to be false for
all future hardware, including future iPads?

> Source/WebCore/testing/Internals.mm:127
> +	   RELEASE_ASSERT_NOT_REACHED();

Seems peculiar to put RELEASE_ASSERT in an internals function, since they are
present only for testing.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:378
> +	   static const char* ioKitClasses[] = {

One more const here would be good, after the *. Or constexpr before the whole
thing.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:388
> +	       "AGXCommandQueue",
> +	       "AGXDevice",
> +	       "AGXSharedUserClient",
> +	       "IOAccelContext",
> +	       "IOAccelDevice",
> +	       "IOAccelSharedUserClient",
> +	       "IOAccelSubmitter2",
> +	       "IOAccelContext2",
> +	       "IOAccelDevice2",
> +	       "IOAccelSharedUserClient2"

Why not sort alphabetically instead of having them almost sorted?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:300
> +    for (size_t i = 0, size =
parameters.dynamicIOKitExtensionHandles.size(); i < size; ++i)

Modern for loop would be good here.

    for (auto& handle : parameters.dynamicIOKitExtensionHandles)

> Tools/TestWebKitAPI/Tests/WebKit/AGXCompilerService.mm:37
> +    WKRetainPtr<WKContextRef> context =
adoptWK(TestWebKitAPI::Util::createContextForInjectedBundleTest("InternalsInjec
tedBundleTest"));

auto