Summary: | [iOS] Deny iokit open access to graphics related classes | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, bfulgham, cdumez, cmarcelo, darin, ews-watchlist, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=219012 https://bugs.webkit.org/show_bug.cgi?id=225004 |
||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2020-04-16 12:15:10 PDT
Created attachment 396696 [details]
Patch
Created attachment 396702 [details]
Patch
Created attachment 396707 [details]
Patch
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. |