Thank you, Serguei!
I updated the webrev with this minor change in the comment. Webrev: http://cr.openjdk.java.net/~dtitov/8197387/webrev.04 Issue: https://bugs.openjdk.java.net/browse/JDK-8197387 Best regards, Daniil From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com> Date: Thursday, May 31, 2018 at 10:33 AM To: Daniil Titov <daniil.x.ti...@oracle.com>, Thomas Stüfe <thomas.stu...@gmail.com>, "serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net" <serviceability-dev@openjdk.java.net> Cc: <ppc-aix-port-...@openjdk.java.net> Subject: Re: RFR 8197387: jcmd started by "root" must be allowed to access all VM processes Hi Daniil, It looks good to me. A minor comment: http://cr.openjdk.java.net/~dtitov/8197387/webrev.03/src/hotspot/os/posix/os_posix.hpp.udiff.html + // Returns true if given uid is effective uid or if given uid is root. + static bool matches_effective_uid_or_root(uid_t uid); What about to simplify the comment above? : // Returns true if given uid is effective or root uid. Thanks, Serguei On 5/30/18 12:08, Daniil Titov wrote: 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.stu...@gmail.com> wrote: 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 > > > > > > > > > > > > > > >
