Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1957893002:
Avoid initializing child processes with an invalid parent pipe (Closed)
|
Created:
4 years, 7 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 7 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@2661 Target Ref:
refs/pending/branch-heads/2661 Project:
chromium Visibility:
Public. |
DescriptionAvoid initializing child processes with an invalid parent pipe
Elevated child processes cannot receive duplicated handles from
the browser in the same way that other child processes can. If
an elevated utility process attempts to initialize Mojo IPC, it
will CHECK fail when trying to use an invalid handle as a result
of this limitation.
In M51+ we no longer attempt to initialize Mojo IPC in elevated
utility processes, but this change in behavior was developed
over several moderately complex patches.
In order to avoid merging said patches into M50, this avoids
the crash in bug 607677 by allowing initialization to fail
silently instead of CHECKing.
This silent failure has no interesting side effects, as Mojo
IPC is not yet used in any elevated utility process.
BUG=607677
R=govind@google.com
TBR=amistry@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/ec1542863ff8f48a729f50aea14894b7998fd6e1
Patch Set 1 #
Messages
Total messages: 14 (5 generated)
Description was changed from ========== Avoid initializing child processes with an invalid parent pipe Elevated child processes cannot receive duplicated handles from the browser in the same way that other child processes can. If an elevated utility process attempts to initialize Mojo IPC, it will CHECK fail at some by trying to use an invalid handle as a result of this limitation. In M51+ we no longer attempt to initialize Mojo IPC in elevated utility processes, but this change in behavior was developed over several moderately complex patches. In order to avoid merging said patches into M50, this avoids the crash in bug 607677 by allowing initialization to fail silently instead of CHECKing. This silent failure has no interesting side effects, as Mojo IPC is not yet used in any elevated utility process. BUG=607677 ========== to ========== Avoid initializing child processes with an invalid parent pipe Elevated child processes cannot receive duplicated handles from the browser in the same way that other child processes can. If an elevated utility process attempts to initialize Mojo IPC, it will CHECK fail when trying to use an invalid handle as a result of this limitation. In M51+ we no longer attempt to initialize Mojo IPC in elevated utility processes, but this change in behavior was developed over several moderately complex patches. In order to avoid merging said patches into M50, this avoids the crash in bug 607677 by allowing initialization to fail silently instead of CHECKing. This silent failure has no interesting side effects, as Mojo IPC is not yet used in any elevated utility process. BUG=607677 ==========
rockot@chromium.org changed reviewers: + govind@chromium.org
Hi govind@, could you please approve this change to land directly in 2661? This is a very low-risk change which fixes the crash in bug 607677. Thanks!
+ Tinazh (Chrome Desktop M50 TPM) Thank you rockot@. I'm kind of new this direct merge approval. Usually for Stable the bar is very high, CL has to be landed in trunk, baked/verified in Canary before we approve merge to Stable. I understand this is a very low-risk change which fixes the crash in bug 607677 (M50 Stable blocker bug) and we're VERY close to M50 stable candidate cut. So adding tinazh@ for her approval. Thank you, Krishna On Sat, May 7, 2016 at 2:07 PM, <rockot@chromium.org> wrote: > Reviewers: govind1 > CL: https://codereview.chromium.org/1957893002/ > > Message: > Hi govind@, could you please approve this change to land directly in 2661? > > This is a very low-risk change which fixes the crash in bug 607677. > > Thanks! > > Description: > Avoid initializing child processes with an invalid parent pipe > > Elevated child processes cannot receive duplicated handles from > the browser in the same way that other child processes can. If > an elevated utility process attempts to initialize Mojo IPC, it > will CHECK fail when trying to use an invalid handle as a result > of this limitation. > > In M51+ we no longer attempt to initialize Mojo IPC in elevated > utility processes, but this change in behavior was developed > over several moderately complex patches. > > In order to avoid merging said patches into M50, this avoids > the crash in bug 607677 by allowing initialization to fail > silently instead of CHECKing. > > This silent failure has no interesting side effects, as Mojo > IPC is not yet used in any elevated utility process. > > BUG=607677 > > Base URL: https://chromium.googlesource.com/chromium/src.git@2661 > > Affected files (+2, -1 lines): > M mojo/edk/system/core.cc > > > Index: mojo/edk/system/core.cc > diff --git a/mojo/edk/system/core.cc b/mojo/edk/system/core.cc > index > 69c918c0be8b50df111abb11646daac1a610a3b9..d3543061003fb035d5b9d2f88b677aeae43fdc21 > 100644 > --- a/mojo/edk/system/core.cc > +++ b/mojo/edk/system/core.cc > @@ -87,7 +87,8 @@ void Core::AddChild(base::ProcessHandle process_handle, > } > > void Core::InitChild(ScopedPlatformHandle platform_handle) { > - GetNodeController()->ConnectToParent(std::move(platform_handle)); > + if (platform_handle.is_valid()) > + GetNodeController()->ConnectToParent(std::move(platform_handle)); > } > > MojoHandle Core::AddDispatcher(scoped_refptr<Dispatcher> dispatcher) { > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Tina, please see rockot@'s update in bug https://bugs.chromium.org/p/chromium/issues/detail?id=607677#c40. This will help you understand why CL needs to be directly landed in M50 branch 2661. Thank you. On Sat, May 7, 2016 at 2:41 PM, Krishna Govind <govind@google.com> wrote: > + Tinazh (Chrome Desktop M50 TPM) > > Thank you rockot@. I'm kind of new this direct merge approval. Usually > for Stable the bar is very high, CL has to be landed in trunk, > baked/verified in Canary before we approve merge to Stable. I understand > this is a very low-risk change which fixes the crash in bug 607677 (M50 > Stable blocker bug) and we're VERY close to M50 stable candidate cut. So > adding tinazh@ for her approval. > > Thank you, > Krishna > > > > > On Sat, May 7, 2016 at 2:07 PM, <rockot@chromium.org> wrote: > >> Reviewers: govind1 >> CL: https://codereview.chromium.org/1957893002/ >> >> Message: >> Hi govind@, could you please approve this change to land directly in >> 2661? >> >> This is a very low-risk change which fixes the crash in bug 607677. >> >> Thanks! >> >> Description: >> Avoid initializing child processes with an invalid parent pipe >> >> Elevated child processes cannot receive duplicated handles from >> the browser in the same way that other child processes can. If >> an elevated utility process attempts to initialize Mojo IPC, it >> will CHECK fail when trying to use an invalid handle as a result >> of this limitation. >> >> In M51+ we no longer attempt to initialize Mojo IPC in elevated >> utility processes, but this change in behavior was developed >> over several moderately complex patches. >> >> In order to avoid merging said patches into M50, this avoids >> the crash in bug 607677 by allowing initialization to fail >> silently instead of CHECKing. >> >> This silent failure has no interesting side effects, as Mojo >> IPC is not yet used in any elevated utility process. >> >> BUG=607677 >> >> Base URL: https://chromium.googlesource.com/chromium/src.git@2661 >> >> Affected files (+2, -1 lines): >> M mojo/edk/system/core.cc >> >> >> Index: mojo/edk/system/core.cc >> diff --git a/mojo/edk/system/core.cc b/mojo/edk/system/core.cc >> index >> 69c918c0be8b50df111abb11646daac1a610a3b9..d3543061003fb035d5b9d2f88b677aeae43fdc21 >> 100644 >> --- a/mojo/edk/system/core.cc >> +++ b/mojo/edk/system/core.cc >> @@ -87,7 +87,8 @@ void Core::AddChild(base::ProcessHandle process_handle, >> } >> >> void Core::InitChild(ScopedPlatformHandle platform_handle) { >> - GetNodeController()->ConnectToParent(std::move(platform_handle)); >> + if (platform_handle.is_valid()) >> + GetNodeController()->ConnectToParent(std::move(platform_handle)); >> } >> >> MojoHandle Core::AddDispatcher(scoped_refptr<Dispatcher> dispatcher) { >> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Given the situation, sgtm to land the change directly, pls trigger a build to verify to mitigate the risk. Thanks! On May 7, 2016 2:50 PM, "Krishna Govind" <govind@google.com> wrote: > Tina, please see rockot@'s update in bug > https://bugs.chromium.org/p/chromium/issues/detail?id=607677#c40. This > will help you understand why CL needs to be directly landed in M50 branch > 2661. Thank you. > > On Sat, May 7, 2016 at 2:41 PM, Krishna Govind <govind@google.com> wrote: > >> + Tinazh (Chrome Desktop M50 TPM) >> >> Thank you rockot@. I'm kind of new this direct merge approval. Usually >> for Stable the bar is very high, CL has to be landed in trunk, >> baked/verified in Canary before we approve merge to Stable. I understand >> this is a very low-risk change which fixes the crash in bug 607677 (M50 >> Stable blocker bug) and we're VERY close to M50 stable candidate cut. So >> adding tinazh@ for her approval. >> >> Thank you, >> Krishna >> >> >> >> >> On Sat, May 7, 2016 at 2:07 PM, <rockot@chromium.org> wrote: >> >>> Reviewers: govind1 >>> CL: https://codereview.chromium.org/1957893002/ >>> >>> Message: >>> Hi govind@, could you please approve this change to land directly in >>> 2661? >>> >>> This is a very low-risk change which fixes the crash in bug 607677. >>> >>> Thanks! >>> >>> Description: >>> Avoid initializing child processes with an invalid parent pipe >>> >>> Elevated child processes cannot receive duplicated handles from >>> the browser in the same way that other child processes can. If >>> an elevated utility process attempts to initialize Mojo IPC, it >>> will CHECK fail when trying to use an invalid handle as a result >>> of this limitation. >>> >>> In M51+ we no longer attempt to initialize Mojo IPC in elevated >>> utility processes, but this change in behavior was developed >>> over several moderately complex patches. >>> >>> In order to avoid merging said patches into M50, this avoids >>> the crash in bug 607677 by allowing initialization to fail >>> silently instead of CHECKing. >>> >>> This silent failure has no interesting side effects, as Mojo >>> IPC is not yet used in any elevated utility process. >>> >>> BUG=607677 >>> >>> Base URL: https://chromium.googlesource.com/chromium/src.git@2661 >>> >>> Affected files (+2, -1 lines): >>> M mojo/edk/system/core.cc >>> >>> >>> Index: mojo/edk/system/core.cc >>> diff --git a/mojo/edk/system/core.cc b/mojo/edk/system/core.cc >>> index >>> 69c918c0be8b50df111abb11646daac1a610a3b9..d3543061003fb035d5b9d2f88b677aeae43fdc21 >>> 100644 >>> --- a/mojo/edk/system/core.cc >>> +++ b/mojo/edk/system/core.cc >>> @@ -87,7 +87,8 @@ void Core::AddChild(base::ProcessHandle process_handle, >>> } >>> >>> void Core::InitChild(ScopedPlatformHandle platform_handle) { >>> - GetNodeController()->ConnectToParent(std::move(platform_handle)); >>> + if (platform_handle.is_valid()) >>> + GetNodeController()->ConnectToParent(std::move(platform_handle)); >>> } >>> >>> MojoHandle Core::AddDispatcher(scoped_refptr<Dispatcher> dispatcher) { >>> >>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Avoid initializing child processes with an invalid parent pipe Elevated child processes cannot receive duplicated handles from the browser in the same way that other child processes can. If an elevated utility process attempts to initialize Mojo IPC, it will CHECK fail when trying to use an invalid handle as a result of this limitation. In M51+ we no longer attempt to initialize Mojo IPC in elevated utility processes, but this change in behavior was developed over several moderately complex patches. In order to avoid merging said patches into M50, this avoids the crash in bug 607677 by allowing initialization to fail silently instead of CHECKing. This silent failure has no interesting side effects, as Mojo IPC is not yet used in any elevated utility process. BUG=607677 ========== to ========== Avoid initializing child processes with an invalid parent pipe Elevated child processes cannot receive duplicated handles from the browser in the same way that other child processes can. If an elevated utility process attempts to initialize Mojo IPC, it will CHECK fail when trying to use an invalid handle as a result of this limitation. In M51+ we no longer attempt to initialize Mojo IPC in elevated utility processes, but this change in behavior was developed over several moderately complex patches. In order to avoid merging said patches into M50, this avoids the crash in bug 607677 by allowing initialization to fail silently instead of CHECKing. This silent failure has no interesting side effects, as Mojo IPC is not yet used in any elevated utility process. BUG=607677 TBR=amistry@chromium.org ==========
On 2016/05/07 23:20:47, chromium-reviews wrote: > Given the situation, sgtm to land the change directly, pls trigger a build > to verify to mitigate the risk. Thanks! > On May 7, 2016 2:50 PM, "Krishna Govind" <mailto:govind@google.com> wrote: > > > Tina, please see rockot@'s update in bug > > https://bugs.chromium.org/p/chromium/issues/detail?id=607677#c40. This > > will help you understand why CL needs to be directly landed in M50 branch > > 2661. Thank you. > > > > On Sat, May 7, 2016 at 2:41 PM, Krishna Govind <mailto:govind@google.com> wrote: > > > >> + Tinazh (Chrome Desktop M50 TPM) > >> > >> Thank you rockot@. I'm kind of new this direct merge approval. Usually > >> for Stable the bar is very high, CL has to be landed in trunk, > >> baked/verified in Canary before we approve merge to Stable. I understand > >> this is a very low-risk change which fixes the crash in bug 607677 (M50 > >> Stable blocker bug) and we're VERY close to M50 stable candidate cut. So > >> adding tinazh@ for her approval. > >> > >> Thank you, > >> Krishna > >> > >> > >> > >> > >> On Sat, May 7, 2016 at 2:07 PM, <mailto:rockot@chromium.org> wrote: > >> > >>> Reviewers: govind1 > >>> CL: https://codereview.chromium.org/1957893002/ > >>> > >>> Message: > >>> Hi govind@, could you please approve this change to land directly in > >>> 2661? > >>> > >>> This is a very low-risk change which fixes the crash in bug 607677. > >>> > >>> Thanks! > >>> > >>> Description: > >>> Avoid initializing child processes with an invalid parent pipe > >>> > >>> Elevated child processes cannot receive duplicated handles from > >>> the browser in the same way that other child processes can. If > >>> an elevated utility process attempts to initialize Mojo IPC, it > >>> will CHECK fail when trying to use an invalid handle as a result > >>> of this limitation. > >>> > >>> In M51+ we no longer attempt to initialize Mojo IPC in elevated > >>> utility processes, but this change in behavior was developed > >>> over several moderately complex patches. > >>> > >>> In order to avoid merging said patches into M50, this avoids > >>> the crash in bug 607677 by allowing initialization to fail > >>> silently instead of CHECKing. > >>> > >>> This silent failure has no interesting side effects, as Mojo > >>> IPC is not yet used in any elevated utility process. > >>> > >>> BUG=607677 > >>> > >>> Base URL: https://chromium.googlesource.com/chromium/src.git@2661 > >>> > >>> Affected files (+2, -1 lines): > >>> M mojo/edk/system/core.cc > >>> > >>> > >>> Index: mojo/edk/system/core.cc > >>> diff --git a/mojo/edk/system/core.cc b/mojo/edk/system/core.cc > >>> index > >>> > 69c918c0be8b50df111abb11646daac1a610a3b9..d3543061003fb035d5b9d2f88b677aeae43fdc21 > >>> 100644 > >>> --- a/mojo/edk/system/core.cc > >>> +++ b/mojo/edk/system/core.cc > >>> @@ -87,7 +87,8 @@ void Core::AddChild(base::ProcessHandle process_handle, > >>> } > >>> > >>> void Core::InitChild(ScopedPlatformHandle platform_handle) { > >>> - GetNodeController()->ConnectToParent(std::move(platform_handle)); > >>> + if (platform_handle.is_valid()) > >>> + GetNodeController()->ConnectToParent(std::move(platform_handle)); > >>> } > >>> > >>> MojoHandle Core::AddDispatcher(scoped_refptr<Dispatcher> dispatcher) { > >>> > >>> > >>> > >> > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. LGTM
Thank you Tina. Ken, please go ahead and submit the cl. I will trigger M50 branched 2661 build once your change is landed. On Sat, May 7, 2016 at 4:40 PM, <govind@google.com> wrote: > On 2016/05/07 23:20:47, chromium-reviews wrote: > > Given the situation, sgtm to land the change directly, pls trigger a > build > > to verify to mitigate the risk. Thanks! > > On May 7, 2016 2:50 PM, "Krishna Govind" <mailto:govind@google.com> > wrote: > > > > > Tina, please see rockot@'s update in bug > > > https://bugs.chromium.org/p/chromium/issues/detail?id=607677#c40. This > > > will help you understand why CL needs to be directly landed in M50 > branch > > > 2661. Thank you. > > > > > > On Sat, May 7, 2016 at 2:41 PM, Krishna Govind <mailto: > govind@google.com> > wrote: > > > > > >> + Tinazh (Chrome Desktop M50 TPM) > > >> > > >> Thank you rockot@. I'm kind of new this direct merge approval. > Usually > > >> for Stable the bar is very high, CL has to be landed in trunk, > > >> baked/verified in Canary before we approve merge to Stable. I > understand > > >> this is a very low-risk change which fixes the crash in bug 607677 > (M50 > > >> Stable blocker bug) and we're VERY close to M50 stable candidate cut. > So > > >> adding tinazh@ for her approval. > > >> > > >> Thank you, > > >> Krishna > > >> > > >> > > >> > > >> > > >> On Sat, May 7, 2016 at 2:07 PM, <mailto:rockot@chromium.org> wrote: > > >> > > >>> Reviewers: govind1 > > >>> CL: https://codereview.chromium.org/1957893002/ > > >>> > > >>> Message: > > >>> Hi govind@, could you please approve this change to land directly in > > >>> 2661? > > >>> > > >>> This is a very low-risk change which fixes the crash in bug 607677. > > >>> > > >>> Thanks! > > >>> > > >>> Description: > > >>> Avoid initializing child processes with an invalid parent pipe > > >>> > > >>> Elevated child processes cannot receive duplicated handles from > > >>> the browser in the same way that other child processes can. If > > >>> an elevated utility process attempts to initialize Mojo IPC, it > > >>> will CHECK fail when trying to use an invalid handle as a result > > >>> of this limitation. > > >>> > > >>> In M51+ we no longer attempt to initialize Mojo IPC in elevated > > >>> utility processes, but this change in behavior was developed > > >>> over several moderately complex patches. > > >>> > > >>> In order to avoid merging said patches into M50, this avoids > > >>> the crash in bug 607677 by allowing initialization to fail > > >>> silently instead of CHECKing. > > >>> > > >>> This silent failure has no interesting side effects, as Mojo > > >>> IPC is not yet used in any elevated utility process. > > >>> > > >>> BUG=607677 > > >>> > > >>> Base URL: https://chromium.googlesource.com/chromium/src.git@2661 > > >>> > > >>> Affected files (+2, -1 lines): > > >>> M mojo/edk/system/core.cc > > >>> > > >>> > > >>> Index: mojo/edk/system/core.cc > > >>> diff --git a/mojo/edk/system/core.cc b/mojo/edk/system/core.cc > > >>> index > > >>> > > > > 69c918c0be8b50df111abb11646daac1a610a3b9..d3543061003fb035d5b9d2f88b677aeae43fdc21 > > >>> 100644 > > >>> --- a/mojo/edk/system/core.cc > > >>> +++ b/mojo/edk/system/core.cc > > >>> @@ -87,7 +87,8 @@ void Core::AddChild(base::ProcessHandle > process_handle, > > >>> } > > >>> > > >>> void Core::InitChild(ScopedPlatformHandle platform_handle) { > > >>> - GetNodeController()->ConnectToParent(std::move(platform_handle)); > > >>> + if (platform_handle.is_valid()) > > >>> + GetNodeController()->ConnectToParent(std::move(platform_handle)); > > >>> } > > >>> > > >>> MojoHandle Core::AddDispatcher(scoped_refptr<Dispatcher> dispatcher) > { > > >>> > > >>> > > >>> > > >> > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > LGTM > > https://codereview.chromium.org/1957893002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
rockot@chromium.org changed reviewers: + govind@google.com
I can't seem to land this CL. Getting authentication errors for https://chromium.googlesource.com. Can I not land this from my chromium.org account?
On 2016/05/08 00:07:18, Ken Rockot wrote: > I can't seem to land this CL. Getting authentication errors for > https://chromium.googlesource.com. Can I not land this from my http://chromium.org > account? What kind of authentication errors you get, Ken, it could be fixable if you google moma around? Not sure what side effect of not landing it from chromium.org account, but if the authentication errors not easy fix (esp. over a weekend), you can give it a try if run out of options.
Description was changed from ========== Avoid initializing child processes with an invalid parent pipe Elevated child processes cannot receive duplicated handles from the browser in the same way that other child processes can. If an elevated utility process attempts to initialize Mojo IPC, it will CHECK fail when trying to use an invalid handle as a result of this limitation. In M51+ we no longer attempt to initialize Mojo IPC in elevated utility processes, but this change in behavior was developed over several moderately complex patches. In order to avoid merging said patches into M50, this avoids the crash in bug 607677 by allowing initialization to fail silently instead of CHECKing. This silent failure has no interesting side effects, as Mojo IPC is not yet used in any elevated utility process. BUG=607677 TBR=amistry@chromium.org ========== to ========== Avoid initializing child processes with an invalid parent pipe Elevated child processes cannot receive duplicated handles from the browser in the same way that other child processes can. If an elevated utility process attempts to initialize Mojo IPC, it will CHECK fail when trying to use an invalid handle as a result of this limitation. In M51+ we no longer attempt to initialize Mojo IPC in elevated utility processes, but this change in behavior was developed over several moderately complex patches. In order to avoid merging said patches into M50, this avoids the crash in bug 607677 by allowing initialization to fail silently instead of CHECKing. This silent failure has no interesting side effects, as Mojo IPC is not yet used in any elevated utility process. BUG=607677 R=govind@google.com TBR=amistry@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/ec1542863ff8f48a729f50aea148... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as ec1542863ff8f48a729f50aea14894b7998fd6e1 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
