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-...@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.stu...@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.ti...@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.stu...@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.ti...@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 > > > > > > >
