RFR 8197387: jcmd started by "root" must be allowed to access all VM processes

[Daniil Titov]

Thank you, Thomas, for verifying this!

I checked over this email thread and believe I still need one more reviewer for this fix.

Webrev: http://cr.openjdk.java.net/~dtitov/8197387/webrev.03/
Issue:  https://bugs.openjdk.java.net/browse/JDK-8197387

Best regards,
Daniil

On 5/30/18, 11:16 AM, "Thomas Stüfe" <thomas.stuefe at gmail.com> wrote:

    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
    >     >     >
    >     >     >
    >     >
    >     >
    >     >
    >
    >
    >