Loosk good.
Thanks,
David
On 21/05/2019 1:25 pm, Daniil Titov wrote:
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
>
>
