Deny iokit open access to graphics related classes in the WebContent process on iOS, but issue extensions for these for some devices which still need access to them.
Created attachment 396696 [details] Patch
Created attachment 396702 [details] Patch
Created attachment 396707 [details] Patch
rdar://problem/61897550
Comment on attachment 396707 [details] 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("InternalsInjectedBundleTest")); auto
Created attachment 396717 [details] Patch
(In reply to Darin Adler from comment #5) > Comment on attachment 396707 [details] > 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? > Yes, I believe that this will be the case going forward. > > 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("InternalsInjectedBundleTest")); > > auto Thanks for reviewing! The latest patch addresses all of the issues above.
Created attachment 396724 [details] Patch
Committed r260247: <https://trac.webkit.org/changeset/260247> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396724 [details].
Comment on attachment 396707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396707&action=review > Tools/TestWebKitAPI/Tests/WebKit/AGXCompilerService.mm:28 > +#if WK_HAVE_C_SPI && PLATFORM(IOS) This test is never run: WK_HAVE_C_SPI is only defined when building on Mac.
(In reply to Daniel Bates from comment #10) > Comment on attachment 396707 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396707&action=review > > > Tools/TestWebKitAPI/Tests/WebKit/AGXCompilerService.mm:28 > > +#if WK_HAVE_C_SPI && PLATFORM(IOS) > > This test is never run: WK_HAVE_C_SPI is only defined when building on Mac. Ah, good point! I will look into making this test run on iOS.