On macOS, connections to the preference daemon are established before entering the sandbox. These connections also persist after entering the sandbox and denying access to the preference daemon. There should not be attempts to connect to the preference daemon before entering the sandbox, since these attempts will not be stopped by the sandbox.
Created attachment 402313 [details] Patch
Comment on attachment 402313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402313&action=review > Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:96 > + StringBuilder path; > + path.append([webKit2Bundle resourcePath]); > + path.append("/"); > + path.append("com.apple.WebKit.NetworkProcess.sb"); > + sandboxParameters.setOverrideSandboxProfilePath(path.toString()); This is a job for makeString, not StringBuilder. sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle resourcePath], "/com.apple.WebKit.NetworkProcess.sb")); Or use a local String variable if you like. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:229 > + // We don't need to talk to the dock. Capitalize Dock. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233 > + if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) { > + if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)]) > + [nsApplicationClass _preventDockConnections]; > + } I know this code is pre-existing and only being moved, but this does seem risky — if this method is renamed, moved, or removed this code will just silently stop having any effect. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:589 > + StringBuilder path; > + path.append([webKit2Bundle resourcePath]); > + path.append("/"); > + path.append("com.apple.WebProcess.sb"); > + sandboxParameters.setOverrideSandboxProfilePath(path.toString()); This is a job for makeString, not StringBuilder. sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle resourcePath], "/com.apple.WebKit.WebProcess.sb")); Or use a local String variable if you like.
Created attachment 402329 [details] Patch
(In reply to Darin Adler from comment #2) > Comment on attachment 402313 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402313&action=review > > > Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:96 > > + StringBuilder path; > > + path.append([webKit2Bundle resourcePath]); > > + path.append("/"); > > + path.append("com.apple.WebKit.NetworkProcess.sb"); > > + sandboxParameters.setOverrideSandboxProfilePath(path.toString()); > > This is a job for makeString, not StringBuilder. > > > sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle > resourcePath], "/com.apple.WebKit.NetworkProcess.sb")); > > Or use a local String variable if you like. > Fixed. > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:229 > > + // We don't need to talk to the dock. > > Capitalize Dock. > Done. > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233 > > + if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) { > > + if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)]) > > + [nsApplicationClass _preventDockConnections]; > > + } > > I know this code is pre-existing and only being moved, but this does seem > risky — if this method is renamed, moved, or removed this code will just > silently stop having any effect. > I could be wrong, but it seems this method is supported. > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:589 > > + StringBuilder path; > > + path.append([webKit2Bundle resourcePath]); > > + path.append("/"); > > + path.append("com.apple.WebProcess.sb"); > > + sandboxParameters.setOverrideSandboxProfilePath(path.toString()); > > This is a job for makeString, not StringBuilder. > > > sandboxParameters.setOverrideSandboxProfilePath(makeString([webKit2Bundle > resourcePath], "/com.apple.WebKit.WebProcess.sb")); > > Or use a local String variable if you like. Fixed. Thanks for reviewing!
Comment on attachment 402329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402329&action=review > Source/WebKit/NetworkProcess/mac/NetworkProcessMac.mm:92 > + sandboxParameters.setOverrideSandboxProfilePath(makeString(String([webKitBundle resourcePath]), "/com.apple.WebKit.NetworkProcess.sb")); You should not need String() around [webKitBundle resourcePath]. If you do, that’s a bug in StringConcatenation.h implementation that we should fix.
(In reply to Per Arne Vollan from comment #4) > (In reply to Darin Adler from comment #2) > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233 > > > + if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) { > > > + if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)]) > > > + [nsApplicationClass _preventDockConnections]; > > > + } > > > > I know this code is pre-existing and only being moved, but this does seem > > risky — if this method is renamed, moved, or removed this code will just > > silently stop having any effect. > > I could be wrong, but it seems this method is supported. Why then are we calling it in such an unusual way?
(In reply to Darin Adler from comment #6) > (In reply to Per Arne Vollan from comment #4) > > (In reply to Darin Adler from comment #2) > > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233 > > > > + if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) { > > > > + if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)]) > > > > + [nsApplicationClass _preventDockConnections]; > > > > + } > > > > > > I know this code is pre-existing and only being moved, but this does seem > > > risky — if this method is renamed, moved, or removed this code will just > > > silently stop having any effect. > > > > I could be wrong, but it seems this method is supported. > > Why then are we calling it in such an unusual way? That is a good point! I wonder if it is done this way for it to compile on OS's where this method is not supported?
Created attachment 402341 [details] Patch
(In reply to Per Arne Vollan from comment #7) > (In reply to Darin Adler from comment #6) > > (In reply to Per Arne Vollan from comment #4) > > > (In reply to Darin Adler from comment #2) > > > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:233 > > > > > + if (Class nsApplicationClass = NSClassFromString(@"NSApplication")) { > > > > > + if ([nsApplicationClass respondsToSelector:@selector(_preventDockConnections)]) > > > > > + [nsApplicationClass _preventDockConnections]; > > > > > + } > > > > > > > > I know this code is pre-existing and only being moved, but this does seem > > > > risky — if this method is renamed, moved, or removed this code will just > > > > silently stop having any effect. > > > > > > I could be wrong, but it seems this method is supported. > > > > Why then are we calling it in such an unusual way? > > That is a good point! I wonder if it is done this way for it to compile on > OS's where this method is not supported? Trying out a patch without using respondsToSelector. Thanks for reviewing!
Committed r263295: <https://trac.webkit.org/changeset/263295> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402341 [details].
<rdar://problem/64549175>
This broke setting log channels via -WebCoreLogging
Re-opened since this is blocked by bug 213577
Created attachment 403234 [details] Patch
Committed r263773: <https://trac.webkit.org/changeset/263773> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403234 [details].