Correct buffer size to fix memory corruption

[DanHeidinga]

The analysis in eclipse-openj9/openj9#5696 shows that there
is a out of bounds access that is fixed by increasing
the buffer size by 8. Validated by having the VM
increase all unsafe allocates by 8 and the problem
no longer occurs.

fixes: #79

Signed-off-by: Dan Heidinga daniel_heidinga@ca.ibm.com

[tonyroberts]

Great catch! The VARIANT struct is different sizes for 32 bit and 64 bit builds though, so I think the buffer sized used should be dependent on whether the process is 32 or 64 bits.

[DanHeidinga]

I can update the PR but am having some trouble finding the definition of VARIANT to double check the size on 32 vs 64. Any pointers would be appreciated

[tonyroberts]

Here you go:
https://docs.microsoft.com/en-gb/windows/desktop/api/oaidl/ns-oaidl-tagvariant

There are two pointers in the inner struct in the union (pvRecord and pRecInfo) which I think account for the 8 bytes difference (4 bytes per pointer difference).

thanks!
Tony

[DanHeidinga]

Thanks @tonyroberts. I've updated the PR to check whether the system is 32 or 64 bit.

[tonyroberts]

"os.arch" is probably more portable to other JVMs than sun.arch.data.model (see https://docs.oracle.com/javase/6/docs/api/java/lang/System.html#getProperties()), but other than that looks good to me.

[mpoindexter]

Would it make sense to move the variantSize call out of the constructor and into a static? In some workloads variants get created a lot, so makes sense to do as little as possible per instance, and I don't think the arch will change mid execution.

[DanHeidinga]

I've switched to os.arch and moved the variantSize check into a static final variable.

[DanHeidinga]

Is there anything I need to do to help get this merged?

[tonyroberts]

@DanHeidinga I'm not sure what the current state of this repo is to be honest. It doesn't seem to have much activity, and my PRs have not been merged or commented on.

I'm maintaining another fork here https://github.com/exceljava/com4j with bug fixes that haven't yet been merged into this repo (including yours).