|
|
Created:
10 years, 3 months ago by cpu_(ooo_6.6-7.5) Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionSandbox built-in flash player. Spawn broker
- Now is chrome duty to spawn flash broker. Flash cannot do it by itself on XP
The flash broker is hosted in rundll32.exe. An extra switch is added to the command line
of the plug-in process so flash player can contact its broker.
BUG=50796
TEST=see bug for testing info
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60018
Patch Set 1 #
Total comments: 17
Patch Set 2 : '' #Messages
Total messages: 6 (0 generated)
also forgot to add that I deleted CHROME_FLASH_ROOT that promised before.
A few questions below http://codereview.chromium.org/3432014/diff/1/2 File chrome/common/sandbox_policy.cc (right): http://codereview.chromium.org/3432014/diff/1/2#newcode312 chrome/common/sandbox_policy.cc:312: bool LoadFlashBroker(const FilePath& plugin_path, CommandLine* cmd_line) { Do you think this function really belongs in this file? It does not seem to do much about the sandbox. But i don't have a strong opinion http://codereview.chromium.org/3432014/diff/1/2#newcode313 chrome/common/sandbox_policy.cc:313: i'd delete this empty line http://codereview.chromium.org/3432014/diff/1/2#newcode318 chrome/common/sandbox_policy.cc:318: // Rundll32 cannot handle paths with spaces, so we use the short path. even when it's quoted? http://codereview.chromium.org/3432014/diff/1/2#newcode341 chrome/common/sandbox_policy.cc:341: bool ApplyPolicyForBuiltInFlashPlugin(CommandLine* cmd_line, unused parameter? http://codereview.chromium.org/3432014/diff/1/2#newcode392 chrome/common/sandbox_policy.cc:392: if (!LoadFlashBroker(plugin_path, cmd_line)) { what can cause that to fail? Why not just abort?
drive by. http://codereview.chromium.org/3432014/diff/1/2 File chrome/common/sandbox_policy.cc (right): http://codereview.chromium.org/3432014/diff/1/2#newcode317 chrome/common/sandbox_policy.cc:317: rundll = rundll.AppendASCII("rundll32.exe"); Is it possible to use something more specific? Maybe another Chrome.exe? It's better not to have processed related to us that are not clearly marked as such. http://codereview.chromium.org/3432014/diff/1/2#newcode320 chrome/common/sandbox_policy.cc:320: if (0 == ::GetShortPathNameW(plugin_path.value().c_str(), Do we care about systems that have short names disabled?. We'll see failures from the field. http://codereview.chromium.org/3432014/diff/1/2#newcode327 chrome/common/sandbox_policy.cc:327: base::ProcessHandle ph; nit: ph -> process
Good comments. Updated code. please look again. http://codereview.chromium.org/3432014/diff/1/2 File chrome/common/sandbox_policy.cc (right): http://codereview.chromium.org/3432014/diff/1/2#newcode312 chrome/common/sandbox_policy.cc:312: bool LoadFlashBroker(const FilePath& plugin_path, CommandLine* cmd_line) { On 2010/09/20 17:47:59, nsylvain wrote: > Do you think this function really belongs in this file? It does not seem to do > much about the sandbox. But i don't have a strong opinion I think it does. At least the launching of all processes happens in this file (lines 521, 566). http://codereview.chromium.org/3432014/diff/1/2#newcode313 chrome/common/sandbox_policy.cc:313: On 2010/09/20 17:47:59, nsylvain wrote: > i'd delete this empty line Done. http://codereview.chromium.org/3432014/diff/1/2#newcode317 chrome/common/sandbox_policy.cc:317: rundll = rundll.AppendASCII("rundll32.exe"); On 2010/09/20 18:24:49, rvargas wrote: > Is it possible to use something more specific? Maybe another Chrome.exe? It's > better not to have processed related to us that are not clearly marked as such. Not sure what you mean here. Acrobat reader and media player have random processes as childs. Task manager does not show the relation, you need process explorer for that. http://codereview.chromium.org/3432014/diff/1/2#newcode318 chrome/common/sandbox_policy.cc:318: // Rundll32 cannot handle paths with spaces, so we use the short path. On 2010/09/20 17:47:59, nsylvain wrote: > even when it's quoted? rundll32 does not understand quotes there :( I tried. http://codereview.chromium.org/3432014/diff/1/2#newcode320 chrome/common/sandbox_policy.cc:320: if (0 == ::GetShortPathNameW(plugin_path.value().c_str(), On 2010/09/20 18:24:49, rvargas wrote: > Do we care about systems that have short names disabled?. We'll see failures > from the field. Yes we care. In that case we return false and we bail via line 395 which allows you to run flash with weak policy. Which requires no broker. http://codereview.chromium.org/3432014/diff/1/2#newcode327 chrome/common/sandbox_policy.cc:327: base::ProcessHandle ph; On 2010/09/20 18:24:49, rvargas wrote: > nit: ph -> process Done. http://codereview.chromium.org/3432014/diff/1/2#newcode341 chrome/common/sandbox_policy.cc:341: bool ApplyPolicyForBuiltInFlashPlugin(CommandLine* cmd_line, On 2010/09/20 17:47:59, nsylvain wrote: > unused parameter? Good catch. fixed. http://codereview.chromium.org/3432014/diff/1/2#newcode392 chrome/common/sandbox_policy.cc:392: if (!LoadFlashBroker(plugin_path, cmd_line)) { On 2010/09/20 17:47:59, nsylvain wrote: > what can cause that to fail? Why not just abort? You can disable short names on a volume. It would be crazy to do so but it happens. In that case we still run flash in a weak sandbox so that it does not need a broker.
lg On Mon, Sep 20, 2010 at 4:26 PM, <cpu@chromium.org> wrote: > Good comments. Updated code. please look again. > > > > > http://codereview.chromium.org/3432014/diff/1/2 > File chrome/common/sandbox_policy.cc (right): > > http://codereview.chromium.org/3432014/diff/1/2#newcode312 > chrome/common/sandbox_policy.cc:312: bool LoadFlashBroker(const > FilePath& plugin_path, CommandLine* cmd_line) { > On 2010/09/20 17:47:59, nsylvain wrote: > >> Do you think this function really belongs in this file? It does not >> > seem to do > >> much about the sandbox. But i don't have a strong opinion >> > > I think it does. At least the launching of all processes happens in this > file (lines 521, 566). > > http://codereview.chromium.org/3432014/diff/1/2#newcode313 > chrome/common/sandbox_policy.cc:313: > > On 2010/09/20 17:47:59, nsylvain wrote: > >> i'd delete this empty line >> > > Done. > > > http://codereview.chromium.org/3432014/diff/1/2#newcode317 > chrome/common/sandbox_policy.cc:317: rundll = > rundll.AppendASCII("rundll32.exe"); > On 2010/09/20 18:24:49, rvargas wrote: > >> Is it possible to use something more specific? Maybe another >> > Chrome.exe? It's > >> better not to have processed related to us that are not clearly marked >> > as such. > > Not sure what you mean here. Acrobat reader and media player have random > processes as childs. Task manager does not show the relation, you need > process explorer for that. > > > http://codereview.chromium.org/3432014/diff/1/2#newcode318 > chrome/common/sandbox_policy.cc:318: // Rundll32 cannot handle paths > with spaces, so we use the short path. > On 2010/09/20 17:47:59, nsylvain wrote: > >> even when it's quoted? >> > > rundll32 does not understand quotes there :( I tried. > > > http://codereview.chromium.org/3432014/diff/1/2#newcode320 > chrome/common/sandbox_policy.cc:320: if (0 == > ::GetShortPathNameW(plugin_path.value().c_str(), > On 2010/09/20 18:24:49, rvargas wrote: > >> Do we care about systems that have short names disabled?. We'll see >> > failures > >> from the field. >> > > Yes we care. In that case we return false and we bail via line 395 which > allows you to run flash with weak policy. Which requires no broker. > > > http://codereview.chromium.org/3432014/diff/1/2#newcode327 > chrome/common/sandbox_policy.cc:327: base::ProcessHandle ph; > On 2010/09/20 18:24:49, rvargas wrote: > >> nit: ph -> process >> > > Done. > > > http://codereview.chromium.org/3432014/diff/1/2#newcode341 > chrome/common/sandbox_policy.cc:341: bool > ApplyPolicyForBuiltInFlashPlugin(CommandLine* cmd_line, > On 2010/09/20 17:47:59, nsylvain wrote: > >> unused parameter? >> > > Good catch. fixed. > > > http://codereview.chromium.org/3432014/diff/1/2#newcode392 > chrome/common/sandbox_policy.cc:392: if (!LoadFlashBroker(plugin_path, > cmd_line)) { > On 2010/09/20 17:47:59, nsylvain wrote: > >> what can cause that to fail? Why not just abort? >> > > You can disable short names on a volume. It would be crazy to do so but > it happens. In that case we still run flash in a weak sandbox so that it > does not need a broker. > > > http://codereview.chromium.org/3432014/show >
http://codereview.chromium.org/3432014/diff/1/2 File chrome/common/sandbox_policy.cc (right): http://codereview.chromium.org/3432014/diff/1/2#newcode317 chrome/common/sandbox_policy.cc:317: rundll = rundll.AppendASCII("rundll32.exe"); On 2010/09/20 23:26:08, cpu wrote: > On 2010/09/20 18:24:49, rvargas wrote: > > Is it possible to use something more specific? Maybe another Chrome.exe? It's > > better not to have processed related to us that are not clearly marked as > such. > > Not sure what you mean here. Acrobat reader and media player have random > processes as childs. Task manager does not show the relation, you need process > explorer for that. > Now I'm not sure what you mean :). Sure, anybody can have random children processes, but we don't (so far). With task manager it is possible to see all processes that we launch directly (chrome*), and if we are taking the trouble to control what this plugin can do, it seems that we could fix the naming. I just hate rundll32 :) |