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

[David Holmes]

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