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" <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
     >
     >

Reply via email to