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

[Daniil Titov]

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?

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