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

[serguei.spitsyn at oracle.com]

Hi Daniil,

+1

Thanks,
Serguei


On 5/20/19 23:20, David Holmes wrote:
> 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
>>          >
>>          >
>>
>>