Use Unsafe API in more correct way

[alsemenov]

• pass result of Unsafe.staticFieldBase() instead of Lookup.class to conform with specification for Unsafe.getObject() requirements.
• avoid calling Unsafe.getObject() via reflection

Fixes #1672

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[sameb]

Hi @alsemenov -- it looks like the CLA check failed. GitHub sees the email address associated with the PR as "Aleksei Semenov alsemenov@users.noreply.github.com", which likely isn't the actual email. I'm pretty sure the CLA check needs to know the actual email, so it can validate it.

[alsemenov]

Hi @sameb
Thank you for pointing out on my incorrect e-mail address.
I have updated the PR with the correct address.

[sameb]

Both AnonymousClassDefiner and other parts of HiddenClassDefiner still refer to sun.misc.Unsafe via reflection. Updating just this one tiny part to directly access it feels weird, and it also leaves the code messy: GET_OBJECT_METHOD is still defined but now unused.

Additionally, AFAICT, this is effectively reverting the code back to doing what it was doing before 3399615, which was submitted because of #1672. Based on this PR, I'm inclined to agree with @mcculls comment @ #1672 (comment). The TL;DR of that comment is: "The code looks like it was already doing the right thing, except doing it via reflection. It's possible the 'not really an Object' is making the reflective call fail. Using Lookup.class directly avoids the problem."

IIUC, this PR is attempting to revert back to using staticFieldBase instead of Lookup.class, and is avoiding the original problem by not using reflection to do the call. If I'm understanding correctly, can you clarify why using Lookup.class is not a valid solution?

[alsemenov]

Both AnonymousClassDefiner and other parts of HiddenClassDefiner still refer to sun.misc.Unsafe via reflection. Updating just this one tiny part to directly access it feels weird, and it also leaves the code messy: GET_OBJECT_METHOD is still defined but now unused.

Accessing sun.misc.Unsafe via reflection is legal thing. However passing result of Unsafe.staticFieldBase() via reflection is not ok. Because result of Unsafe.staticFieldBase() might be just a "cookie" instead of real java object and is not intended for such usage.

I updated the PR and removed the GET_OBJECT_METHOD. I see no reason to update AnonymousClassDefiner, because it does not use Unsafe.staticFieldBase().

can you clarify why using Lookup.class is not a valid solution?

Let's read descriptions of Unsafe methods:

    /**
     * Fetches a reference value from a given Java variable.
     * @see #getInt(Object, long)
     */
    @ForceInline
    public Object getObject(Object o, long offset) {

   /**
     * Fetches a value from a given Java variable.
     * More specifically, fetches a field or array element within the given
     * object {@code o} at the given offset, or (if {@code o} is null)
     * from the memory address whose numerical value is the given offset.
     * <p>
     * The results are undefined unless one of the following cases is true:
     * <ul>
     * <li>The offset was obtained from {@link #objectFieldOffset} on
     * the {@link java.lang.reflect.Field} of some Java field and the object
     * referred to by {@code o} is of a class compatible with that
     * field's class.
     *
     * <li>The offset and object reference {@code o} (either null or
     * non-null) were both obtained via {@link #staticFieldOffset}
     * and {@link #staticFieldBase} (respectively) from the
     * reflective {@link Field} representation of some Java field.
     *
     * <li>The object referred to by {@code o} is an array, and the offset
     * is an integer of the form {@code B+N*S}, where {@code N} is
     * a valid index into the array, and {@code B} and {@code S} are
     * the values obtained by {@link #arrayBaseOffset} and {@link
     * #arrayIndexScale} (respectively) from the array's class.  The value
     * referred to is the {@code N}<em>th</em> element of the array.
     *
     * </ul>
     * <p>
     * If one of the above cases is true, the call references a specific Java
     * variable (field or array element).  However, the results are undefined
     * if that variable is not in fact of the type returned by this method.
     * <p>
     ... 
     */
    @ForceInline
    public int getInt(Object o, long offset) {

We clearly fall into second case, which requires both object and offset to be obtained from staticFieldBase and staticFieldOffset. If, for example, Unsafe.staticFieldBase() returns some kind of a wrapper around Lookup.class, then Unsafe.getObject() would expect to get back that wrapper along with offset. Passing bare Lookup.class will break the Unsafe.getObject()

[mcculls]

Before #1672 we were following the second case and used both staticFieldBase and staticFieldOffset - using this results in a JVM crash with Azul Prime, so as a workaround we used the class as a replacement for staticFieldBase.

I wasn't able to test directly with Azul Prime because I don't have a license for it - but I found other OSS projects, such as Apache/Felix, were using the class directly for staticFieldBase. Since I couldn't find similar crash reports in those OSS projects involving Azul Prime and there are several products that build on Apache/Felix, I inferred that this would be an acceptable workaround for #1672

If this is not the case and Azul Prime expects staticFieldBase (ie. different to the class) then I suggest we revert 3399615 so that we are at least consistent with the official usage in the javadoc.

I'd then suggest that a fix be made to Azul Prime so that it throws an exception instead of crashing the JVM - this would at least allow our code to fall back to alternative approaches.

[alsemenov]

The sole reason for Azul Prime crash is passing result of Unsafe.staticFieldBase() via reflection. Eventually reflection is replaced by on-the-fly generated method, which casts first argument to Object. Since the value is not real java object the cast cause VM to crash.

I'd then suggest that a fix be made to Azul Prime so that it throws an exception instead of crashing the JVM - this would at least allow our code to fall back to alternative approaches.

According to the Unsafe methods descriptions:

...
     * @throws RuntimeException No defined exceptions are thrown, not even
     *         {@link NullPointerException}
     */
    @ForceInline
    public int getInt(Object o, long offset) {

So we are not allowed to throw here.
Also additional check inside Unsafe.getObject() will cause undesirable performance degradation.

[alsemenov]

If this is not the case and Azul Prime expects staticFieldBase (ie. different to the class) then I suggest we revert 3399615 so that we are at least consistent with the official usage in the javadoc.

Yes, Azul Prime expects result of Unsafe.staticFieldBase() which is even not a real java object. Reverting 3399615 is fine with me. Should I submit another PR?

[mcculls]

@sameb yes if you're ok with direct references (guarded by try/catch) that's fine with me - the only potential issue is if/when javac restricts access to that class, but that would only limit the JDK used to build Guice and not the runtime version...

[sameb]

👍. Let's go with direct references throughout, which IIUC should also resolve this issue (if reverting back to the staticFieldBase). Could you (@mcculls) send a PR doing that?

[sameb]

@mcculls: I took a look at this locally and have a working version which I'll push out shortly. Turns out we still needed the reflection for defineAnonymousClass (b/c it's removed after 17+). Sorry for asking you to do work and then doing it myself!

[azul-jf]

Hi @mcculls, did you download Prime and test #1722 with it?

[sameb]

FYI another user reported the latest code works with Azul, see #1732 (reply in thread).

[alsemenov]

Changed the PR to revert 3399615

[sameb]

FYI I have an alternate version of this fix @ #1722 that I'll submit soon. (The failed checks are just an unrelated race condition in the test I need to fix.)