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, )) {
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, )) {
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, )) {
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: 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: 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
