8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows

[Daniil Titov]

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-bounces at openjdk.java.net on behalf of daniil.x.titov at 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.holmes at 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
        > 
        >