Call to Structure#toArray with a zero length under Netapi32Util.java

[trevormaggs]

Provide complete information about the problem

1. Version of JNA and related jars
   JNA version 5.2.0 - jna-5.2.0.jar and jna-platform-5.2.0.jar

2. Version and vendor of the java virtual machine
   java version "1.8.0_181"
   Java(TM) SE Runtime Environment (build 1.8.0_181-b13)
   Java HotSpot(TM) 64-Bit Server VM (build 25.181-b13, mixed mode)

3. Operating system
   Windows 10

4. System architecture (CPU type, bitness of the JVM)
   Xeon X3430 @ 2,40GHz and 64 bit JVM

5. Complete description of the problem
   The bug was discovered in the getUserLocalGroups method under Netapi32Util.java. When running in Line 369:

LOCALGROUP_USERS_INFO_0[] lgroups = (LOCALGROUP_USERS_INFO_0[]) lgroup.toArray(entriesread.getValue()) where entriesread.getValue() has a zero value. In the other word, it essentially sends 0 to Structure#toArray, which it then throws an ArrayIndexOutOfBoundsException exception, because the array size is zero.

6. Steps to reproduce.

I try to be as simple as possible. Only 2 lines need to be added.

1 - Add if (entriesread.getValue() > 0) before LOCALGROUP_USERS_INFO_0 lgroup = new LOCALGROUP_USERS_INFO_0(bufptr.getValue());

The "if" condition above prevents a zero sized array.

2 - Add return new Group[0]; at the end of the method. This is to safely return an empty array rather than null. See a complete listing with the solution in place below. I apologise for not following the JDK style, but you can maintain your usual style with the solution added. Thanks.

    public static Group[] getUserLocalGroups(String userName, String serverName)
    {
        PointerByReference bufptr = new PointerByReference();
        IntByReference entriesread = new IntByReference();
        IntByReference totalentries = new IntByReference();

        try
        {
            int rc = Netapi32.INSTANCE.NetUserGetLocalGroups(serverName, userName, 0, 0, bufptr, LMCons.MAX_PREFERRED_LENGTH, entriesread, totalentries);

            if (rc != LMErr.NERR_Success)
            {
                throw new Win32Exception(rc);
            }

            if (entriesread.getValue() > 0) <-- Prevents a zero sized array 
            {
                LOCALGROUP_USERS_INFO_0 lgroup = new LOCALGROUP_USERS_INFO_0(bufptr.getValue());
                LOCALGROUP_USERS_INFO_0[] lgroups = (LOCALGROUP_USERS_INFO_0[]) lgroup.toArray(entriesread.getValue());

                ArrayList<Group> result = new ArrayList<Group>();

                for (LOCALGROUP_USERS_INFO_0 lgpi : lgroups)
                {
                    LocalGroup lgp = new LocalGroup();

                    if (lgpi.lgrui0_name != null)
                    {
                        lgp.name = lgpi.lgrui0_name.toString();
                    }

                    result.add(lgp);
                }

                return result.toArray(new Group[0]);
            }
        }

        finally
        {
            if (bufptr.getValue() != Pointer.NULL)
            {
                int rc = Netapi32.INSTANCE.NetApiBufferFree(bufptr.getValue());

                if (rc != LMErr.NERR_Success)
                {
                    throw new Win32Exception(rc);
                }
            }
        }

        return new Group[0];  <-- Safely returns an empty array rather than null
    }

I realised the getUserLocalGroups method is not the only one that is affected. Below is the list of other methods, I believe also need to be corrected to eliminate the bug.

getLocalGroups(String serverName)
getGlobalGroups(String serverName)
getUsers(String serverName)
getUserGroups(String userName, String serverName)
getDomainTrusts(String serverName)

Let me know how it goes. Thanks.

[matthiasblaesing]

Please have a look here: #1092 does that match your expectations? Could you please test it?

[trevormaggs]

Hi Matthias,

Thanks for introducing the bug fixes. I have tested every method except for getDomainTrusts as I do not have any domain controllers here, but the solution still looks fine and I am confident it is fine.

Testing showed there was no ArrayIndexOutOfBoundsException exception raised. All good.

Just one comment, I think you need to remove the redundant toString() part in the getGlobalGroups method.

ie from lgp.name = lgpi.grpi1_name**.toString()**; to lgp.name = lgpi.grpi1_name;

Does it mean the solution will be released as part of version 5.3.2 later?

Thank you once again for the fixes.

Trevor

[matthiasblaesing]

Hi Trevor,

I don't see the toString() call you bean. Could it be, that you are looking at the original version? Your hint made me aware, that the null checks are not needed anymore, as these only protected the toString() calls.

I pushed an additional commit, that removes these redundant calls. Could you please have another look?

For 5.3.2 - I'd like to let the dust settle a bit, if nothing new comes up, sure, maybe someone adds features, than it will be released as part of 5.4.0. 5.3.1 was a quick release because 5.3.0 introduced regressions. This problem is old. The situation would change, if you indicate, that you are facing the problem currently.

Greetings

Matthias

[trevormaggs]

Hi Matthias,

The toString() is still visible in getGlobalGroups.

See Line 276 in https://github.com/java-native-access/jna/blob/0b098dbbb12bf912f438421e6837b3739dafa0f6/contrib/platform/src/com/sun/jna/platform/win32/Netapi32Util.java

That is fine to let the dust to settle first. It makes sense. Besides, I am in no hurry to use it anyway. Thanks again.

Trevor

[matthiasblaesing]

Hi Trevor,

the PR consists of three commits and you referenced the first one. The two followup commits remove the toString calls (a568454) and the null checks (1bfce08). The three commits combined form the PR and the combined diff is visible here:

https://github.com/java-native-access/jna/pull/1092/files

Greetings

Matthias

[trevormaggs]

Hi Matthias,

Ah of course. I apologise for my oversight. Yes, the code is all good. Release it whenever you are ready. There is no hurry really. Thank you once again for fixing the bugs. Appreciated.

Cheers,

Trevor Maggs

[matthiasblaesing]

Thank you Trevor - merged to master.