8365609: Fix several potential NULL native pointer dereferences in the desktop module #26799

savoptik · 2025-08-15T13:04:35Z

The defect has been detected and confirmed in the function OGLBlitToSurfaceViaTexture() located in the file src/java.desktop/share/native/common/java2d/opengl/OGLBlitLoops.c with static code analysis. This defect can potentially lead to a null pointer dereference.

The pointer pf is dereferenced in line 324 without checking for nullptr, although earlier in line 274 the same pointer is checked for nullptr, which indicates that it can be null.

In the same file, line 551 calls OGLBlitToSurfaceViaTexture() from line 263, where NULL is passed in place of pf.
All other calls are fine.

Also, another function with a similar issue from the same file, OGLBlitSwToTexture() from line 396.

In src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c gtk3_load()
The pointer fp_glib_check_version can be null, but it is dereferenced without any check. Although in the same file, for example, line 280 contains a check, this check does not lead to termination of execution.

In src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c SplashDecodeGif()
The pointer colorMap is dereferenced after it has been checked against nullptr in lines 151 and 206. Moreover, between these checks and the mentioned location (line 282), the pointer is not modified in any way.

According to this comment, this PR contains fixes for similar cases in other places.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8365609: Fix several potential NULL native pointer dereferences in the desktop module (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26799/head:pull/26799
$ git checkout pull/26799

Update a local copy of the PR:
$ git checkout pull/26799
$ git pull https://git.openjdk.org/jdk.git pull/26799/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26799

View PR using the GUI difftool:
$ git pr show -t 26799

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26799.diff

Using Webrev

Link to Webrev Comment

…mon/java2d/opengl/OGLBlitLoops.c OGLBlitToSurfaceViaTexture() Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Artem Semenov <[email protected]>.

…a2d/opengl/OGLBlitLoops.c OGLBlitSwToTexture() Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Artem Semenov <[email protected]>.

…creen/splashscreen_gif.c SplashDecodeGif() Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Artem Semenov <[email protected]>.

…/awt/gtk3_interface.c gtk3_load() Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Artem Semenov <[email protected]>.

bridgekeeper · 2025-08-15T13:05:09Z

👋 Welcome back asemenov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-08-15T13:07:12Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-08-15T13:08:24Z

@savoptik The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-08-15T13:11:21Z

Webrevs

mrserb · 2025-08-15T17:05:18Z

The pointer pf is dereferenced in line 324 without checking for nullptr, although earlier in line 274 the same pointer is checked for nullptr, which indicates that it can be null.

It is better first to confirm whether this pointer can actually be NULL. If it cannot then remove the unnecessary earlier NULL check.

prrace · 2025-08-20T05:27:03Z

There seem to be un-related changes in this PR. Please check.

savoptik · 2025-08-20T11:39:28Z

The pointer pf is dereferenced in line 324 without checking for nullptr, although earlier in line 274 the same pointer is checked for nullptr, which indicates that it can be null.

It is better first to confirm whether this pointer can actually be NULL. If it cannot then remove the unnecessary earlier NULL check.

In the same file, line 551 calls OGLBlitToSurfaceViaTexture() from line 263, where NULL is passed in place of pf.
All other calls are fine.

Also, another function with a similar issue from the same file, OGLBlitSwToTexture() from line 396, is called with an existing pf.
It is not possible to remove the first check as you suggest…
Since that line calculates the value:

jboolean adjustAlpha = (pf != NULL && !pf->hasAlpha);

I suggest keeping the NULL check for OGLBlitToSurfaceViaTexture() at line 263, while for OGLBlitSwToTexture() at line 396, the check can be replaced with an assert.

savoptik · 2025-08-20T12:06:19Z

There seem to be un-related changes in this PR. Please check.

In this PR, they asked not to create multiple tickets for the same issues… even if they are in different files…

However, it’s not difficult for me to distribute the fixes across several tickets into different subcomponents if you think it’s necessary to do so.

prrace · 2025-08-21T20:59:33Z

There seem to be un-related changes in this PR. Please check.

In this PR, they asked not to create multiple tickets for the same issues… even if they are in different files…

However, it’s not difficult for me to distribute the fixes across several tickets into different subcomponents if you think it’s necessary to do so.

I see. From the bug subject I would never have guessed it touched GTK and gif decoding.
And you expended about 3 sentences in the description on the OGL one and didn't discuss the others.
They deserve some discussion too.

And then the JBS issue and the PR should have a title change .. something like this
"Fix several potential NULL native pointer dereferences in the desktop module".

prrace · 2025-08-21T22:36:02Z

src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c

-            fp_g_uuid_string_is_valid = //since: 2.52
-                    dl_symbol("g_uuid_string_is_valid");
-            fp_g_variant_print = dl_symbol("g_variant_print"); // since 2.24
+        if (fp_glib_check_version != NULL) {


I don't know why we don't treat failing to find this symbol as a fatal error like we do for others such as this first one. dl_symbol will do a longjmp
fp_gtk_check_version = dl_symbol("gtk_check_version");

but for this one we just clear the error.
/* GLib */
fp_glib_check_version = dlsym(gtk3_libhandle, "glib_check_version");
if (!fp_glib_check_version) {
dlerror();
}

@azvegint any idea ?

I don't know why we don't treat failing to find this symbol as a fatal error like we do for others such as this first one.

It looks like the glib_check_version was added but never used. The first instance of its use appears to be the added check for the methods used for Screencast. And it was overlooked that the fp_glib_check_version can be null.

The glib_check_version is available since 2.6, and the 2.6.0 was released in 2004, so I guess we can safely replace the dlsym with the dl_symbol.
This will be better than remembering to protect it with a null check if we decide to use glib_check_version later somewhere else.

- fp_glib_check_version = dlsym(gtk3_libhandle, "glib_check_version"); - if (!fp_glib_check_version) { - dlerror(); - } + fp_glib_check_version = dl_symbol("glib_check_version");

So that would be the proper fix here, not the proposed one.

I don't know why we don't treat failing to find this symbol as a fatal error like we do for others such as this first one.

It looks like the glib_check_version was added but never used. The first instance of its use appears to be the added check for the methods used for Screencast. And it was overlooked that the fp_glib_check_version can be null.

The glib_check_version is available since 2.6, and the 2.6.0 was released in 2004, so I guess we can safely replace the dlsym with the dl_symbol. This will be better than remembering to protect it with a null check if we decide to use glib_check_version later somewhere else.

- fp_glib_check_version = dlsym(gtk3_libhandle, "glib_check_version"); - if (!fp_glib_check_version) { - dlerror(); - } + fp_glib_check_version = dl_symbol("glib_check_version");

done

prrace · 2025-08-21T22:37:14Z

src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c

@@ -279,7 +279,8 @@ SplashDecodeGif(Splash * splash, GifFileType * gif)
                ImageRect dstRect;
                rgbquad_t fillColor = 0;                        // 0 is transparent

-                if (transparentColor < 0) {
+                if (((colorMap != NULL) && (colorMap->Colors != NULL)) &&


I can't easily verify that this is an impossible place to be if colorMap == null
so I guess this is OK

I can't easily verify that this is an impossible place to be if colorMap == null so I guess this is OK

Do you mean that this check should be removed or kept?

…awt_xawt/awt/gtk3_interface.c gtk3_load()" This reverts commit a369e3a.

…terface.c gtk3_load()

savoptik · 2025-08-26T07:38:53Z

There seem to be un-related changes in this PR. Please check.

In this PR, they asked not to create multiple tickets for the same issues… even if they are in different files…
However, it’s not difficult for me to distribute the fixes across several tickets into different subcomponents if you think it’s necessary to do so.

I see. From the bug subject I would never have guessed it touched GTK and gif decoding. And you expended about 3 sentences in the description on the OGL one and didn't discuss the others. They deserve some discussion too.

And then the JBS issue and the PR should have a title change .. something like this "Fix several potential NULL native pointer dereferences in the desktop module".

done

src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c

azvegint · 2025-08-28T12:44:58Z

src/java.desktop/share/native/common/java2d/opengl/OGLBlitLoops.c

+                                        if (pf != NULL) {
+                                                            if (slowPath) {


The current indentation does seem to be off.

GLvoid *pSrc = PtrCoord(srcInfo->rasBase, sx, srcInfo->pixelStride, sy, srcInfo->scanStride); if (pf) { if (slowPath) { jint tmph = sh; while (tmph > 0) { ...

The same applies to the other added if statement.

Co-authored-by: Aleksandr Zvegintsev <[email protected]>

prsadhuk · 2025-08-29T08:02:30Z

src/java.desktop/share/native/common/java2d/opengl/OGLBlitLoops.c

-
-        while (height > 0) {
-            j2d_glTexSubImage2D(dstOps->textureTarget, 0,
+    if (pf != NULL) {


jdk/src/java.desktop/share/native/common/java2d/opengl/OGLBlitLoops.c

Line 651 in c96d09a

OGLBlitSwToTexture(&srcInfo, &pf, dstOps,

OGLBlitSwToTexture is called with PixelFormat defined as

jdk/src/java.desktop/share/native/common/java2d/opengl/OGLSurfaceData.c

Lines 52 to 76 in c96d09a

OGLPixelFormat PixelFormats[] = {

{ GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV,

4, 1, 0, }, /* 0 - IntArgb */

{ GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV,

4, 1, 1, }, /* 1 - IntArgbPre */

{ GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV,

4, 0, 1, }, /* 2 - IntRgb */

{ GL_RGBA, GL_UNSIGNED_INT_8_8_8_8,

4, 0, 1, }, /* 3 - IntRgbx */

{ GL_RGBA, GL_UNSIGNED_INT_8_8_8_8_REV,

4, 0, 1, }, /* 4 - IntBgr */

{ GL_BGRA, GL_UNSIGNED_INT_8_8_8_8,

4, 0, 1, }, /* 5 - IntBgrx */

{ GL_RGB, GL_UNSIGNED_SHORT_5_6_5,

2, 0, 1, }, /* 6 - Ushort565Rgb */

{ GL_BGRA, GL_UNSIGNED_SHORT_1_5_5_5_REV,

2, 0, 1, }, /* 7 - Ushort555Rgb */

{ GL_RGBA, GL_UNSIGNED_SHORT_5_5_5_1,

2, 0, 1, }, /* 8 - Ushort555Rgbx*/

{ GL_LUMINANCE, GL_UNSIGNED_BYTE,

1, 0, 1, }, /* 9 - ByteGray */

{ GL_LUMINANCE, GL_UNSIGNED_SHORT,

2, 0, 1, }, /*10 - UshortGray */

{ GL_BGR, GL_UNSIGNED_BYTE,

1, 0, 1, }, /*11 - ThreeByteBgr */};

which is not null so not sure why pf will ever be null? Did you see it null anytime?

This was discussed already. There's another call site that passes NULL.

Oh this is a different function. Not the one previously discussed, looks like it was added to the PR later.
This case at least currently cannot be called with NULL for the reason above.

prsadhuk · 2025-08-29T08:11:19Z

src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c

+                    colorMap->Colors &&
+                    transparentColor < 0) {
                    fillColor= MAKE_QUAD_GIF(
                        colorMap->Colors[gif->SBackGroundColor], 0xff);


Dont we need to check for colorMap->Colors here too

jdk/src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c

Line 208 in c96d09a

colorMapBuf[i] = MAKE_QUAD_GIF(colorMap->Colors[i], 0xff);

The problem isn't the Colors member, it is colorMap. I'm not sure the new check for Colors is necessary.
If you look at giflib's allocator for this it will never return a colorMap without Colors allocated and the giflib code itself assumes that this is true.

prrace · 2025-08-29T16:30:40Z

src/java.desktop/share/native/common/java2d/opengl/OGLBlitLoops.c

-                    jint tmph = sh;
-                    while (tmph > 0) {
-                        j2d_glTexSubImage2D(GL_TEXTURE_2D, 0,
+                if (pf != NULL) {


I don't think any of these changes in this file are necessary.
pf is null IFF sw_surface == JNI_FALSE
and iin that case, we never end the loop where pf is needed.
Put another way, if you look at the call sites one
is
pf != null, srcOps == NULL, sw_surface == FALSE
the other is
pf == null, srcOps != NULL, sw_surface == TRUE

and you can see that srcOps would have the same theoretical issue except that it would be a bug to call it with the wrong value of sw_surface.
So revert all changes in this file.

This reverts commit c96d09a.

…tive/common/java2d/opengl/OGLBlitLoops.c OGLBlitToSurfaceViaTexture()" This reverts commit ff9825c.

…mmon/java2d/opengl/OGLBlitLoops.c OGLBlitSwToTexture()" This reverts commit c4b8712.

savoptik and others added 4 commits August 15, 2025 16:02

8365609 Null pointer dereference in src/java.desktop/share/native/com…

ff9825c

…mon/java2d/opengl/OGLBlitLoops.c OGLBlitToSurfaceViaTexture() Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Artem Semenov <[email protected]>.

The same issue is present in src/java.desktop/share/native/common/jav…

c4b8712

…a2d/opengl/OGLBlitLoops.c OGLBlitSwToTexture() Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Artem Semenov <[email protected]>.

The same issue is present in src/java.desktop/share/native/libsplashs…

e74b2bc

…creen/splashscreen_gif.c SplashDecodeGif() Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Artem Semenov <[email protected]>.

The same issue is present in src/java.desktop/unix/native/libawt_xawt…

a369e3a

…/awt/gtk3_interface.c gtk3_load() Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Artem Semenov <[email protected]>.

openjdk bot added client [email protected] rfr Pull request is ready for review labels Aug 15, 2025

prrace reviewed Aug 21, 2025

View reviewed changes

savoptik changed the title ~~8365609: Null pointer dereference in src/java.desktop/share/native/common/java2d/opengl/OGLBlitLoops.c OGLBlitToSurfaceViaTexture()~~ 8365609: Fix several potential NULL native pointer dereferences in the desktop module Aug 22, 2025

savoptik added 2 commits August 25, 2025 13:49

Revert "The same issue is present in src/java.desktop/unix/native/lib…

92ab60e

…awt_xawt/awt/gtk3_interface.c gtk3_load()" This reverts commit a369e3a.

Updated fix fore src/java.desktop/unix/native/libawt_xawt/awt/gtk3_in…

a730765

…terface.c gtk3_load()

azvegint reviewed Aug 28, 2025

View reviewed changes

savoptik and others added 2 commits August 28, 2025 15:56

Update src/java.desktop/share/native/libsplashscreen/splashscreen_gif.c

f0a276f

Co-authored-by: Aleksandr Zvegintsev <[email protected]>

Fixed indentation

c96d09a

prsadhuk reviewed Aug 29, 2025

View reviewed changes

prrace reviewed Aug 29, 2025

View reviewed changes

savoptik added 3 commits September 1, 2025 13:17

Revert "Fixed indentation"

761afd3

This reverts commit c96d09a.

Revert "8365609 Null pointer dereference in src/java.desktop/share/na…

cf92998

…tive/common/java2d/opengl/OGLBlitLoops.c OGLBlitToSurfaceViaTexture()" This reverts commit ff9825c.

Revert "The same issue is present in src/java.desktop/share/native/co…

1a5a401

…mmon/java2d/opengl/OGLBlitLoops.c OGLBlitSwToTexture()" This reverts commit c4b8712.

	OGLPixelFormat PixelFormats[] = {
	{ GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV,
	4, 1, 0, }, /* 0 - IntArgb */
	{ GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV,
	4, 1, 1, }, /* 1 - IntArgbPre */
	{ GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV,
	4, 0, 1, }, /* 2 - IntRgb */
	{ GL_RGBA, GL_UNSIGNED_INT_8_8_8_8,
	4, 0, 1, }, /* 3 - IntRgbx */
	{ GL_RGBA, GL_UNSIGNED_INT_8_8_8_8_REV,
	4, 0, 1, }, /* 4 - IntBgr */
	{ GL_BGRA, GL_UNSIGNED_INT_8_8_8_8,
	4, 0, 1, }, /* 5 - IntBgrx */
	{ GL_RGB, GL_UNSIGNED_SHORT_5_6_5,
	2, 0, 1, }, /* 6 - Ushort565Rgb */
	{ GL_BGRA, GL_UNSIGNED_SHORT_1_5_5_5_REV,
	2, 0, 1, }, /* 7 - Ushort555Rgb */
	{ GL_RGBA, GL_UNSIGNED_SHORT_5_5_5_1,
	2, 0, 1, }, /* 8 - Ushort555Rgbx*/
	{ GL_LUMINANCE, GL_UNSIGNED_BYTE,
	1, 0, 1, }, /* 9 - ByteGray */
	{ GL_LUMINANCE, GL_UNSIGNED_SHORT,
	2, 0, 1, }, /10 - UshortGray /
	{ GL_BGR, GL_UNSIGNED_BYTE,
	1, 0, 1, }, /11 - ThreeByteBgr /};

8365609: Fix several potential NULL native pointer dereferences in the desktop module #26799

Are you sure you want to change the base?

8365609: Fix several potential NULL native pointer dereferences in the desktop module #26799

Conversation

savoptik commented Aug 15, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Aug 15, 2025

Uh oh!

openjdk bot commented Aug 15, 2025

Uh oh!

openjdk bot commented Aug 15, 2025

Uh oh!

mlbridge bot commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

mrserb commented Aug 15, 2025

Uh oh!

prrace commented Aug 20, 2025

Uh oh!

savoptik commented Aug 20, 2025

Uh oh!

savoptik commented Aug 20, 2025

Uh oh!

prrace commented Aug 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savoptik commented Aug 26, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savoptik commented Aug 15, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 15, 2025 •

edited

Loading