RFR 8197387: jcmd started by "root" must be allowed to access all VM processes
Thomas Stüfe
thomas.stuefe at gmail.com
Wed May 30 18:16:16 UTC 2018
On Wed, May 30, 2018 at 7:27 PM, Daniil Titov <daniil.x.titov at oracle.com> wrote:
> Hi Thomas,
>
> Thank you for the review. Just in in case I put an updated webrev with suggested changes at http://cr.openjdk.java.net/~dtitov/8197387/webrev.03/ .
>
> I am also including ppc-aix-port-dev at openjdk.java.net mail list since the changes affect AIX native code. I did these AIX changes myself and I need to get them verified before the push.
>
> May I ask someone who has access to an AIX machine try this patch to ensure that AIX build is fine?
>
AIX builds fine.
Thanks, Thomas
> Webrev: http://cr.openjdk.java.net/~dtitov/8197387/webrev.03/
> Issue: https://bugs.openjdk.java.net/browse/JDK-8197387
>
> Thanks a lot!
>
> Best regards,
> Daniil
>
> On 5/29/18, 9:54 PM, "Thomas Stüfe" <thomas.stuefe at gmail.com> wrote:
>
> Hi Daniil,
>
> Looks fine. Small nits:
>
> - please add short comments to os_posix.hpp on the new prototypes. At
> least on matches_effective_uid_and_gid_or_root:
> e.g. // Returns true if either given uid is effective uid and given
> gid is effective gid, or if given uid is root.
>
> - src/hotspot/os/posix/os_posix.cpp:
>
> +bool os::Posix::matches_effective_uid_and_gid_or_root(uid_t uid, gid_t gid) {
> + return is_root(uid) || ( geteuid() == uid && getegid() == gid);
> +}
> +
> please remove extra space before geteuid()
>
> If you change above points, I do not need another webrev. Reviewed.
>
> --
>
> Btw: I wonder why we find it necessary in the hotspot to check for
> both uid AND gid to match effective uid and effective gid? Seems
> strange. But since this logic is not touched by your change, your
> change is okay.
>
> Best Regards, Thomas
>
>
>
> On Tue, May 29, 2018 at 11:33 PM, Daniil Titov
> <daniil.x.titov at oracle.com> wrote:
> > Hi Thomas,
> >
> > Please review a new version of the fix that includes the changes suggested.
> >
> > Webrev: http://cr.openjdk.java.net/~dtitov/8197387/webrev.02/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8197387
> >
> > Thank you,
> > Daniil
> >
> >
> > On 5/24/18, 10:51 PM, "Thomas Stüfe" <thomas.stuefe at gmail.com> wrote:
> >
> > Hi Daniil,
> >
> > here is my review:
> >
> > - Like Roger I would prefer to have the uid checks factored out. At
> > least for the hotspot coding, I do not know where to put it in jdk
> > coding. For the hotspot parts, I would add something like:
> >
> > os::Posix::is_root(uid_t uid) ;
> > os::Posix::matches_effective_uid_or_root(uid_t uid) // return
> > isroot(uid) || uid == geteuid
> > os::Posix::matches_effective_group_id(gid_t gid) // return gid == getegid
> >
> > to os_posix.hpp/os_posix.cpp
> >
> > Other than that, the changes make sense.
> >
> > Kind Regards, Thomas
> >
> >
> >
> >
> > On Thu, May 24, 2018 at 3:11 AM, Daniil Titov <daniil.x.titov at oracle.com> wrote:
> > > Please review the changes that fix JDK-8197387.
> > >
> > > There are 2 problems here:
> > > 1. JVM ignores .attach_pid<pid> file if it is owned by the user different from the one that owns this JVM process
> > > 2. jcmd checks that .java_pid<pid> socket is owned by the same user that runs jcmd and reports an error otherwise
> > >
> > > The fix relaxes these checks to allow jcmd started by "root" (UID = 0) access JVMs started by another users.
> > >
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8197387
> > > Webrev: http://cr.openjdk.java.net/~dtitov/8197387/webrev.01/
> > >
> > > Best regards,
> > > Daniil
> > >
> > >
> >
> >
> >
>
>
>
More information about the ppc-aix-port-dev
mailing list