Re: [12u] RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
+1 --alex On 05/24/2019 02:55, serguei.spit...@oracle.com wrote: Hi Daniil, The fix has been applied cleanly. LGTM++ Thanks, Serguei On 5/23/19 18:22, Daniil Titov wrote: Please review the backport of this fix to JDK 12. The JDK 12 changes applied mostly smoothly, but one hunk in make/test/JtregNativeJdk.gmk didn't apply because of changed context lines. That's the only difference. Webrev: http://cr.openjdk.java.net/~dtitov/backports/jdk12u/8214545/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 Main webrev: http://cr.openjdk.java.net/~dtitov/8214545/webrev.03 Testing: sun/management/jmxremote/bootstrap, jdk_svc, tier1, tier2 and tier3 tests succeeded. Thanks! --Daniil
Re: [12u] RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
Hi Daniil, The fix has been applied cleanly. LGTM++ Thanks, Serguei On 5/23/19 18:22, Daniil Titov wrote: Please review the backport of this fix to JDK 12. The JDK 12 changes applied mostly smoothly, but one hunk in make/test/JtregNativeJdk.gmk didn't apply because of changed context lines. That's the only difference. Webrev: http://cr.openjdk.java.net/~dtitov/backports/jdk12u/8214545/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 Main webrev: http://cr.openjdk.java.net/~dtitov/8214545/webrev.03 Testing: sun/management/jmxremote/bootstrap, jdk_svc, tier1, tier2 and tier3 tests succeeded. Thanks! --Daniil
Re: [12u] RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
Looks good. Thanks, David On 24/05/2019 11:22 am, Daniil Titov wrote: Please review the backport of this fix to JDK 12. The JDK 12 changes applied mostly smoothly, but one hunk in make/test/JtregNativeJdk.gmk didn't apply because of changed context lines. That's the only difference. Webrev: http://cr.openjdk.java.net/~dtitov/backports/jdk12u/8214545/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 Main webrev: http://cr.openjdk.java.net/~dtitov/8214545/webrev.03 Testing: sun/management/jmxremote/bootstrap, jdk_svc, tier1, tier2 and tier3 tests succeeded. Thanks! --Daniil
[12u] RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
Please review the backport of this fix to JDK 12. The JDK 12 changes applied mostly smoothly, but one hunk in make/test/JtregNativeJdk.gmk didn't apply because of changed context lines. That's the only difference. Webrev: http://cr.openjdk.java.net/~dtitov/backports/jdk12u/8214545/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 Main webrev: http://cr.openjdk.java.net/~dtitov/8214545/webrev.03 Testing: sun/management/jmxremote/bootstrap, jdk_svc, tier1, tier2 and tier3 tests succeeded. Thanks! --Daniil
Re: RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
Hi Daniil, I realize now that the test for -f rather than -x was likely because in the source bundle the exe file couldn't actually have the execute permission. So -f was correct then while -x should I hope be correct now. In which case you should be able to get rid of: chmod ug+x $REVOKEALL as well. But we'd need to be sure the execute bit is kept on the binary after its built and shipped around to other test machines. If in doubt restore the -f. Otherwise the updates look good. Thanks, David On 21/05/2019 11:02 am, Daniil Titov 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" 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 > >
Re: RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
LGTM --alex On 05/20/2019 18:02, Daniil Titov 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" 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 > >
Re: RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
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" 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 > >
Re: RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
Build changes look good. /Erik On 2019-05-19 17:43, David Holmes 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
Re: jmx-dev RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
On 20.05.2019 13:13, Daniel Fuchs wrote: Hi, On 20/05/2019 01:43, David Holmes wrote: Webrev: http://cr.openjdk.java.net/~dtitov/8214545 Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 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. Looks like `i` tries to count the entries that were not modified - so the fact that it was not incremented before `continue` looks like an oversight. I'd say that Daniil is right. There is iterating over list and changing it same time. It is common to iterate backward in such case to simplify logic. But this code is Ok for me too. I believe Alan wrote that tool - he may be able to confirm ;-) That said - if we could do the same thing in java as Alan suggests and replace these shell scripts with java that might be a big win! best regards, -- daniel
Re: RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
The change is good. Thank you! regards, Boris On 20.05.2019 3:43, David Holmes 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
Re: jmx-dev RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
Hi, On 20/05/2019 01:43, David Holmes wrote: Webrev: http://cr.openjdk.java.net/~dtitov/8214545 Bug: https://bugs.openjdk.java.net/browse/JDK-8214545 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. Looks like `i` tries to count the entries that were not modified - so the fact that it was not incremented before `continue` looks like an oversight. I'd say that Daniil is right. I believe Alan wrote that tool - he may be able to confirm ;-) That said - if we could do the same thing in java as Alan suggests and replace these shell scripts with java that might be a big win! best regards, -- daniel
Re: RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
On 19/05/2019 22:12, 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 Have you looked at replacing the tool with code that uses the java.nio.file API? You can edit ACLs with that API. -Alan
Re: RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
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
RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows
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 Thanks! --Daniil