Repository navigation
Call to Structure#toArray with a zero length under Netapi32Util.java #1091
Description
Provide complete information about the problem
-
Version of JNA and related jars
JNA version 5.2.0 - jna-5.2.0.jar and jna-platform-5.2.0.jar -
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) -
Operating system
Windows 10 -
System architecture (CPU type, bitness of the JVM)
Xeon X3430 @ 2,40GHz and 64 bit JVM -
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.
- 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.
Activity
Check target number to be greater than zero, before calling `Structur…
matthiasblaesing commentedon May 5, 2019
Please have a look here: #1092 does that match your expectations? Could you please test it?
trevormaggs commentedon May 6, 2019
Hi Matthias,
Thanks for introducing the bug fixes. I have tested every method except for
getDomainTrustsas 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
ArrayIndexOutOfBoundsExceptionexception raised. All good.Just one comment, I think you need to remove the redundant toString() part in the
getGlobalGroupsmethod.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 commentedon May 6, 2019
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 thetoString()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 commentedon May 8, 2019
Hi Matthias,
The
toString()is still visible ingetGlobalGroups.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 commentedon May 8, 2019
Hi Trevor,
the PR consists of three commits and you referenced the first one. The two followup commits remove the
toStringcalls (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 commentedon May 9, 2019
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 commentedon May 10, 2019
Thank you Trevor - merged to master.