Please review un updated version of the previous change that also removes unnecessary line
chmod ug+x $REVOKEALL from test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh Webrev: http://cr.openjdk.java.net/~dtitov/8214545/webrev.03 Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 Thanks! --Daniil On 5/20/19, 6:02 PM, "serviceability-dev on behalf of Daniil Titov" <serviceability-dev-boun...@openjdk.java.net on behalf of daniil.x.ti...@oracle.com> wrote: Please review a new version of the fix that includes the changes David suggested. > The count-- is obvious as it is the loop counter, but it is far from > clear to me that i++ is correct. I don't fully understand the logic We need to increment i on line 354: 353 if (((ACCESS_ALLOWED_ACE *)ace)->Header.AceType != ACCESS_ALLOWED_ACE_TYPE) { 354 i++; 355 count--; 356 continue; 357 } since the code iterates over all ACE entries for a given file and deletes ones that grant non-owner access to the file. i is the index of the current ACE entry in the ACL structure. The current ACE entry is retrieved at the beginning of the loop: 349 if (!GetAce(acl, i, &ace)) { and the index is always incremented at the end of the loop unless the current entry is deleted. 382 if (!deleted) { 383 str = getSIDString(sid); 384 if (str != NULL) { 385 printf("ALLOW %s (access mask=%x)\n", str, access->Mask); 386 free(str); 387 } 388 389 /* onto the next ACE */ 390 i++; 391 } 392 count--; I also created a new issue to replace revokeall.exe with Java code as Alan suggested : https://bugs.openjdk.java.net/browse/JDK-8224255 Webrev: http://cr.openjdk.java.net/~dtitov/8214545/webrev.02 Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 Thanks! --Daniil On 5/19/19, 5:43 PM, "David Holmes" <david.hol...@oracle.com> wrote: Hi Daniil, cc: Boris and Erik J. On 20/05/2019 7:12 am, Daniil Titov wrote: > Please review the change that fixes the failure of sun/management/jmxremote/bootstrap JMX tests on Windows platform. While running, these tests invoke revokeall.exe utility and this utility hangs. > > The problem here is that invokeall.exe goes into an endless loop while iterating over Access Control Entries (ACE) for a given file if it encounters at least one ACE with the type different from ACCESS_ALLOWED_ACE_TYPE. > > The change fixes this problem. It also removes revokeall.exe binary from the repository and changes the makefile to get it built instead. > > Tier1, tier2, tier3, jdk_svc, and sun/management/jmxremote/bootstrap tests succeeded in Mach5. > > Webrev: http://cr.openjdk.java.net/~dtitov/8214545 > Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 I knew this seemed very familiar ... Boris had a fix for this a few weeks ago under JDK-8220581. Similar but not identical to yours - see below. Though getting rid of the exe from the repo is a good idea (thanks Erik!). A few comments test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh Pre-existing: ! REVOKEALL="$TESTNATIVEPATH/revokeall.exe" if [ ! -f "$REVOKEALL" ] ; then I would expect a -x test not -f. --- test/jdk/sun/management/windows/README The first copyright year should be 2004. 25 This directory contains the source and the binary version Delete "and the binary version". --- test/jdk/sun/management/windows/exerevokeall.c Pre-existing: 31 * file - suitable for NT/2000/XP only. Please delete everything after "file". 355 i++; 356 count--; The count-- is obvious as it is the loop counter, but it is far from clear to me that i++ is correct. I don't fully understand the logic but i is only incremented under very specific conditions. If you rewrote the code to avoid the use of the continue then i would not be modified except where it currently is. Thanks, David ----- > Thanks! > --Daniil > >
