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

[Thomas Stüfe]

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