Summary: | [iOS] Block the accessibility server when accessibility is not enabled. | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, bfulgham, cdumez, cfleizach, cmarcelo, commit-queue, dbates, ews-watchlist, Hironori.Fujii, ryanhaddad, tsavell, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2019-03-05 14:47:47 PST
might depend how we're checking accessibility. this could be a bit risky given all the ways AX can be turned on Created attachment 363694 [details]
Patch
Comment on attachment 363694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363694&action=review > Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56 > + SandboxExtension::Handle handle; are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification (In reply to chris fleizach from comment #5) > Comment on attachment 363694 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363694&action=review > > > Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56 > > + SandboxExtension::Handle handle; > > are you handling when AX turns off after process launched by monitoring > kAXSApplicationAccessibilityEnabledNotification Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off. Thanks for reviewing! Comment on attachment 363694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363694&action=review >>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56 >>> + SandboxExtension::Handle handle; >> >> are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification > > Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off. > > Thanks for reviewing! I was thinking if AX turns on after the web process has already started (In reply to chris fleizach from comment #7) > Comment on attachment 363694 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363694&action=review > > >>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56 > >>> + SandboxExtension::Handle handle; > >> > >> are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification > > > > Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off. > > > > Thanks for reviewing! > > I was thinking if AX turns on after the web process has already started Ah, right. The AX server will be unblocked (if AX is on) whenever Safari becomes the front application. Is that sufficient? Comment on attachment 363694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363694&action=review >>>>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56 >>>>> + SandboxExtension::Handle handle; >>>> >>>> are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification >>> >>> Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off. >>> >>> Thanks for reviewing! >> >> I was thinking if AX turns on after the web process has already started > > Ah, right. The AX server will be unblocked (if AX is on) whenever Safari becomes the front application. Is that sufficient? no I don't think so. if you launch safari, then you turn on AX, this will fail.. you need to listen for that notification too and unblock Created attachment 363765 [details]
Patch
(In reply to chris fleizach from comment #9) > Comment on attachment 363694 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363694&action=review > > >>>>> Source/WebKit/UIProcess/ios/WebProcessProxyIOS.mm:56 > >>>>> + SandboxExtension::Handle handle; > >>>> > >>>> are you handling when AX turns off after process launched by monitoring kAXSApplicationAccessibilityEnabledNotification > >>> > >>> Would you prefer to revoke the mach extension when AX is turned off? For simplicity, I don't think it is strictly needed to revoke the extension when AX is being turned off, since the main purpose of this patch is to block the AX server, when AX is already off. > >>> > >>> Thanks for reviewing! > >> > >> I was thinking if AX turns on after the web process has already started > > > > Ah, right. The AX server will be unblocked (if AX is on) whenever Safari becomes the front application. Is that sufficient? > > no I don't think so. if you launch safari, then you turn on AX, this will > fail.. you need to listen for that notification too and unblock Thanks, Chris! I have updated the patch. Attachment 363765 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363765&action=review looks ok from ax perspective. > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:431 > + WebProcessProxy* process = m_processes[i].get(); could probably reduce this to one line m_processes[I]->unblockAccessibilityServerIfNeeded(); Created attachment 363769 [details]
Patch
Attachment 363769 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
ERROR: Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:432: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 2 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 363771 [details]
Patch
Attachment 363771 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
LGTM, but I'm not a WK2 reviewer Comment on attachment 363771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363771&action=review > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:93 > + return sandbox_extension_issue_mach_to_process_by_pid("com.apple.webkit.extension.mach", path, 0, pid.value()); Should this be "com.apple.webkit.extensions.mach"_s, since it's a String being passed? > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:429 > + m_accessibilityEnabledObserver = [[NSNotificationCenter defaultCenter] addObserverForName:(__bridge id)kAXSApplicationAccessibilityEnabledNotification object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *note) { Should we add an UNUSED_PARAM(note) here? iOS build failure: /Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:431:108: error: use of undeclared identifier 'kAXSApplicationAccessibilityEnabledNotification' m_accessibilityEnabledObserver = [[NSNotificationCenter defaultCenter] addObserverForName:(__bridge id)kAXSApplicationAccessibilityEnabledNotification object:nil queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification *note) { ^ 1 error generated. Created attachment 363775 [details]
Patch
Created attachment 363777 [details]
Patch
Thanks for reviewing, all! I have updated the patch. Attachment 363777 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363777&action=review > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:65 > + LOG_ERROR("Could not create a sandbox extension for '%s', errno = %d", m_token, errno); Perhaps this should an early return? > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:69 > + return m_handle != -1; Is m_handle == 0 a valid response? It didn't used to be. (In reply to Brent Fulgham from comment #25) > Comment on attachment 363777 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363777&action=review > > > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:65 > > + LOG_ERROR("Could not create a sandbox extension for '%s', errno = %d", m_token, errno); > > Perhaps this should an early return? > > > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:69 > > + return m_handle != -1; > > Is m_handle == 0 a valid response? It didn't used to be. According to the documentation, the function returns -1 on failure, but I don't think I have seen a return value of 0. Perhaps we should change this to 'return m_handle > 0'? Comment on attachment 363777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363777&action=review >>> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:65 >>> + LOG_ERROR("Could not create a sandbox extension for '%s', errno = %d", m_token, errno); >> >> Perhaps this should an early return? > > According to the documentation, the function returns -1 on failure, but I don't think I have seen a return value of 0. Perhaps we should change this to 'return m_handle > 0'? I suggest you do an early return here with 'false', and leave the "return m_handle" below unchanged from existing. Created attachment 363882 [details]
Patch
(In reply to Brent Fulgham from comment #27) > Comment on attachment 363777 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363777&action=review > > >>> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:65 > >>> + LOG_ERROR("Could not create a sandbox extension for '%s', errno = %d", m_token, errno); > >> > >> Perhaps this should an early return? > > > > According to the documentation, the function returns -1 on failure, but I don't think I have seen a return value of 0. Perhaps we should change this to 'return m_handle > 0'? > > I suggest you do an early return here with 'false', and leave the "return > m_handle" below unchanged from existing. Sounds good, thanks! I have updated the patch. Attachment 363882 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363882 [details]
Patch
Looks good! r=me
(In reply to Brent Fulgham from comment #31) > Comment on attachment 363882 [details] > Patch > > Looks good! r=me Thanks for reviewing! Created attachment 364688 [details]
Patch
(In reply to Per Arne Vollan from comment #33) > Created attachment 364688 [details] > Patch Rebased. Attachment 364688 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 364812 [details]
Patch
Attachment 364812 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 364812 [details] Patch Clearing flags on attachment: 364812 Committed r243008: <https://trac.webkit.org/changeset/243008> Reverted r243008 for reason: This revision broke High Sierra builders Committed r243027: <https://trac.webkit.org/changeset/243027> (In reply to Truitt Savell from comment #39) > Reverted r243008 for reason: > > This revision broke High Sierra builders > > Committed r243027: <https://trac.webkit.org/changeset/243027> Undefined symbols for architecture x86_64: "_sandbox_extension_issue_mach_to_process_by_pid", referenced from: WebKit::SandboxExtensionImpl::sandboxExtensionForType(char const*, WebKit::SandboxExtension::Type, WTF::Optional<int>) in UnifiedSource16-mm.o ld: symbol(s) not found for architecture x86_64 https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20%28Build%29/builds/13030/steps/compile-webkit/logs/stdio Created attachment 364905 [details]
Patch
Attachment 364905 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 364910 [details]
Patch
Attachment 364910 [details] did not pass style-queue:
ERROR: Source/WebKit/Platform/spi/ios/AccessibilitySupportSPI.h:44: _AXSApplicationAccessibilityEnabled is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 364910 [details] Patch Clearing flags on attachment: 364910 Committed r243034: <https://trac.webkit.org/changeset/243034> Committed r243054: <https://trac.webkit.org/changeset/243054> |