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

[David Holmes]

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