8373474: 2 Unintentional format string defect groups in jabswitch.cpp #28949

DamonGuy · 2025-12-22T05:18:34Z

This issue is not currently causing any problems, but I am adding specifiers to explicitly print these strings. This is to avoid any issues down the line where changes to the lines building the string can cause any accidental formatting. Overall, this should be a harmless update and improves the stability and reliability of printing these strings.

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-8373474: 2 Unintentional format string defect groups in jabswitch.cpp (Bug - P3)

Reviewers

Alexander Zvegintsev (@azvegint - Reviewer) Review applies to b4bef299
Alexey Ivanov (@aivanov-jdk - Reviewer)
Phil Race (@prrace - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28949

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-12-22T05:20:32Z

👋 Welcome back dnguyen! 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-12-22T05:21:53Z

@DamonGuy This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8373474: 2 Unintentional format string defect groups in jabswitch.cpp

Reviewed-by: aivanov, prr, azvegint

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 132 new commits pushed to the master branch:

f5249db: 8374233: Overloaded constructor MountUnmountDisabler(jthread thread) is missed
61cb6d7: 8374232: Comment cleanup in diagnosticCommand.cpp
8d80bac: 8374296: Comment clean up in os_linux.cpp
... and 129 more: https://git.openjdk.org/jdk/compare/b6319f5b42738cc760711a3b8b5d442d14a0ed74...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-12-22T05:22:43Z

@DamonGuy 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-12-22T05:27:09Z

Webrevs

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp

prrace · 2025-12-22T21:38:24Z

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp

-                fprintf(origFile, str);
+                fprintf(origFile,
+                    "assistive_technologies=com.sun.java.accessibility.AccessBridge\n"
+                    "screen_magnifier_present=true\n");


I don't think it is what Alexander meant.
I am not sure doing the above would even resolve the complaint because there's still no format string.

I think he meant it should look like
fprintf(origfile, "%s",
"assistive_technologies=com.sun.java.accessibility.AccessBridge\n"screen_magnifier_present=true\n");
or
fprintf(origfile, "%s",
"assistive_technologies=com.sun.java.accessibility.AccessBridge\n" "screen_magnifier_present=true\n");
if you really want to use the automatic concatenation, but I had to check to be sure it would work so ..

I see your point. I'll leave it as separated again just in case the string literal is updated with anything that can be misinterpreted as a specifier.

I don't think it is what Alexander meant.

Damon understood me correctly. That's what I meant.
#28949 (comment)

I am not sure doing the above would even resolve the complaint because there's still no format string.

It should.

before:

fprintf(origFile, str); // using `str` variable as format string > parfait complains

after:

fprintf(origFile, "assistive_technologies=com.sun.java.accessibility.AccessBridge\n" "screen_magnifier_present=true\n");

Here, we provide a format string(without the format specifiers), not the variable.
It's essentially identical to the code on line 301, printf("Unable to get version info.\n");, parfait didn't complain about that line.

if you really want to use the automatic concatenation, but I had to check to be sure it would work so ..

It is in the standard, so I don't see any reason not to use it:
https://en.cppreference.com/w/cpp/language/string_literal.html#Concatenation

So, in my opinion, the variable str is unnecessary here.

just in case the string literal is updated with anything that can be misinterpreted as a specifier.

I suppose it should be detected during the review process for such a change. Currently, there are no format specifiers being used.

OK fair enough.

Why can't we use fputs?

fputs("assistive_technologies=com.sun.java.accessibility.AccessBridge\n" "screen_magnifier_present=true\n", origFile);

No format strings avoid any possible ambiguity and it's much faster as the string is output verbatim without any additional logic to parse a format string and to process the arguments.

I guess the same comment applies here:

A similar pattern is used elsewhere in this file. And I don't want this fix to become a reworking of this code, because that isn't a goal here.

Thus, fprintf is fine.

Similarly, it could be worth cleaning up the code to simplify the logic by not using formatted output where it's not needed.

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp

aivanov-jdk · 2025-12-23T17:15:22Z

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp

-                fprintf(origFile, str);
+                fprintf(origFile,
+                    "assistive_technologies=com.sun.java.accessibility.AccessBridge\n"
+                    "screen_magnifier_present=true\n");


Why can't we use fputs?

fputs("assistive_technologies=com.sun.java.accessibility.AccessBridge\n" "screen_magnifier_present=true\n", origFile);

No format strings avoid any possible ambiguity and it's much faster as the string is output verbatim without any additional logic to parse a format string and to process the arguments.

aivanov-jdk · 2025-12-23T17:18:22Z

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp

+    printf(
+        "jabswitch %s\n"
+        "jabswitch enables or disables the Java Access Bridge.\n",
+        versionString
+    );


If versionString isn't used for anything else, we can remove the versionString variable and the call to sprintf_s and put the arguments to print the version directly into the printf call.

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp

aivanov-jdk · 2025-12-23T20:33:02Z

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp

-                fprintf(origFile, str);
+                fprintf(origFile,
+                    "assistive_technologies=com.sun.java.accessibility.AccessBridge\n"
+                    "screen_magnifier_present=true\n");


I guess the same comment applies here:

A similar pattern is used elsewhere in this file. And I don't want this fix to become a reworking of this code, because that isn't a goal here.

Thus, fprintf is fine.

Similarly, it could be worth cleaning up the code to simplify the logic by not using formatted output where it's not needed.

DamonGuy · 2025-12-24T00:04:14Z

/integrate

openjdk · 2025-12-24T00:05:12Z

Going to push as commit 72e1e15.
Since your change was applied there have been 132 commits pushed to the master branch:

f5249db: 8374233: Overloaded constructor MountUnmountDisabler(jthread thread) is missed
61cb6d7: 8374232: Comment cleanup in diagnosticCommand.cpp
8d80bac: 8374296: Comment clean up in os_linux.cpp
... and 129 more: https://git.openjdk.org/jdk/compare/b6319f5b42738cc760711a3b8b5d442d14a0ed74...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-12-24T00:05:20Z

@DamonGuy Pushed as commit 72e1e15.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Initial commit

3b1f27c

openjdk bot added the client [email protected] label Dec 22, 2025

openjdk bot added the rfr Pull request is ready for review label Dec 22, 2025

azvegint reviewed Dec 22, 2025

View reviewed changes

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp Outdated Show resolved Hide resolved

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp Outdated Show resolved Hide resolved

Update printf

b4bef29

azvegint approved these changes Dec 22, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 22, 2025

prrace reviewed Dec 22, 2025

View reviewed changes

Revert

ebbafcf

openjdk bot removed the ready Pull request is ready to be integrated label Dec 22, 2025

prrace reviewed Dec 22, 2025

View reviewed changes

src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp Outdated Show resolved Hide resolved

Review comments

e17b09a

aivanov-jdk suggested changes Dec 23, 2025

View reviewed changes

Update copyright year

2cc83b2

aivanov-jdk approved these changes Dec 23, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 23, 2025

prrace approved these changes Dec 23, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 24, 2025

openjdk bot closed this Dec 24, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 24, 2025

8373474: 2 Unintentional format string defect groups in jabswitch.cpp #28949

8373474: 2 Unintentional format string defect groups in jabswitch.cpp #28949

Conversation

DamonGuy commented Dec 22, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Dec 22, 2025

Uh oh!

openjdk bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

prrace Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DamonGuy Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

azvegint Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

prrace Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aivanov-jdk Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aivanov-jdk Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

DamonGuy commented Dec 24, 2025

Uh oh!

openjdk bot commented Dec 24, 2025

Uh oh!

openjdk bot commented Dec 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

DamonGuy commented Dec 22, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Dec 22, 2025 •

edited

Loading

openjdk bot commented Dec 22, 2025 •

edited

Loading

mlbridge bot commented Dec 22, 2025 •

edited

Loading

prrace Dec 22, 2025 •

edited

Loading

aivanov-jdk Dec 23, 2025 •

edited

Loading

aivanov-jdk Dec 23, 2025 •

edited

Loading

aivanov-jdk Dec 23, 2025 •

edited

Loading