On Wed, May 30, 2018 at 7:27 PM, Daniil Titov <daniil.x.ti...@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-...@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.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 > > > > > > > > > > > > > > >
