On Fri, 5 Sep 2025 21:46:22 GMT, Damon Nguyen <[email protected]> wrote:
>> src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c line 1289:
>>
>>> 1287: AWT_UNLOCK ();
>>> 1288:
>>> 1289: if ((*env)->ExceptionCheck(env)) {
>>
>> Is the problem that we are here because bounds == null and one way this is
>> possible is that the call at line 1265 failed? Meaning threw an unlikely
>> exception.
>> If so, maybe that is where we should just "return NULL".
>
> My understanding of the issue is that there is a possible `pendingException`
> on line 1287. This is possible by `AWT_NOFLUSH_UNLOCK_IMPL()` in `awt.h` as
> you previously pointed me to. Seems like setting bounds by the code that was
> previously on lines 1289-1290 was unsafe due to the `AWT_UNLOCK` possibly
> throwing an exception right before it.
>
> I don't think bounds can be null in this area because line 1279 checks for
> this. But if the `AWT_UNLOCK` here is throwing a `pendingException`, I think
> returning null is what should be done here since the same is done on line
> 1297, except this won't be reached in this case until after bounds is
> unsafely set.
>
> I can just `return NULL` instead near line 1265 as you suggested, but from
> what I read, the exception would be at line 1287 (which is after).
You are right - there is a check for !bounds. Although that is likely there
because we might not be in the Xinerama case, not a concern about a pending
exception, although it would also help in that case too.
However if NewObject fails then bounds will be null and there will be a pending
exception. So I think we should return immediately in that case.
But my point is that this bug report is a result of static analysis, so we
should be able to follow that same logic.
Meaning AWT_UNLOCK isn't the source of the exception.
It is rethrowing one it previously copied and cleared
But it only does all of that IF there was such a pending exception.
So I am looking back at where the exception might have come from.
I missed this line
(*env)->ThrowNew(env, exceptionClass, "Illegal screen
index");
that seems likely to be it. If we are throwing an exception because of an
illegal screen index, there is absolutely no point in returning a new object -
the caller won't be able to receive it because they'll get an exception. And
clearing it would be wrong - it is meant to be thrown.
So there should also be a return after that ThrowNew.
Once you've done that we can statically prove there are no possible pending
exceptions and this call to ExceptionCheck isn't necessary.
Basically a fix for this is to return immediately in both cases - failure of
NewObject and ThrowNew.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27110#discussion_r2334443764
