Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8262199: issue in jli args.c #2692

Closed
wants to merge 5 commits into from
Closed

Conversation

@MBaesken
Copy link
Member

@MBaesken MBaesken commented Feb 23, 2021

Sonar reports a finding in args.c, where a file check is done .
Stat performs a check on file, and later fopen is called on the file .

The coding could be slightly rewritten so that the potential issue is removed (however I do not think that it is such a big issue).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2692/head:pull/2692
$ git checkout pull/2692

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 23, 2021

👋 Welcome back mbaesken! 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.

Loading

@openjdk openjdk bot added the rfr label Feb 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 23, 2021

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

  • core-libs

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.

Loading

@openjdk openjdk bot added the core-libs label Feb 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 23, 2021

Loading

Copy link
Contributor

@RealCLanger RealCLanger left a comment

This looks good in general. Do you know whether there's a jtreg test that stresses arg files?

Loading

if (fmt != NULL) JLI_ReportMessage(fmt, arg);
if (fptr != NULL) fclose(fptr);
exit(1);
}

This comment has been hidden.

Loading

Copy link
Contributor

@AlanBateman AlanBateman Feb 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is "optionally report, optionally fclose, and then exit". Have you tried reducing it to reportAndExit and fclose inline in expandArgFile to avoid it doing 3 things?

Loading

Copy link
Member Author

@MBaesken MBaesken Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is "optionally report, optionally fclose, and then exit". Have you tried reducing it to reportAndExit and fclose inline in expandArgFile to avoid it doing 3 things?

hi Alan , thanks for your remark , I did not do that because I would need to copy both lines (fclose + exit) to all callers.
On the other hand I wonder - wouldn't it be possible to avoid the fclose before the exit(1) ?
I assume the closing is done on exit anyway ?

Loading

Copy link
Contributor

@AlanBateman AlanBateman Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds okay to me, the process is exiting, as you say.

Loading

@MBaesken
Copy link
Member Author

@MBaesken MBaesken commented Feb 23, 2021

This looks good in general. Do you know whether there's a jtreg test that stresses arg files?

There are tests dealing with args files at test/jdk/tools/launcher/ , e.g. there is test/jdk/tools/launcher/ArgsFileTest.java .

Best regards, Matthias

Loading

@RealCLanger
Copy link
Contributor

@RealCLanger RealCLanger commented Feb 23, 2021

This looks good in general. Do you know whether there's a jtreg test that stresses arg files?

There are tests dealing with args files at test/jdk/tools/launcher/ , e.g. there is test/jdk/tools/launcher/ArgsFileTest.java .

Best regards, Matthias

OK, I added label noreg-sqe to the bug then.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 23, 2021

@MBaesken 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:

8262199: issue in jli args.c

Reviewed-by: clanger, alanb

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 43 new commits pushed to the master branch:

  • 0a4e710: 8261954: Dependencies: Improve iteration over class hierarchy under context class
  • 722142e: 8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java
  • bcca100: 4710675: JTextArea.setComponentOrientation does not work with correct timing
  • fce5765: 8262433: doclint: reference error in module jdk.incubator.foreign
  • 059ede0: 8262428: doclint warnings in java.xml module
  • 8256517: 8262421: doclint warnings in jdk.compiler module
  • 29c603f: 8262227: Change SystemDictionary::find() to return an InstanceKlass*.
  • 35c0a69: 8262416: ProblemList TestHeapDumpForLargeArray.java due to JDK-8262386
  • 228c285: 8261170: Upgrade to freetype 2.10.4
  • ded96dd: 8139348: Deprecate 3DES and RC4 in Kerberos
  • ... and 33 more: https://git.openjdk.java.net/jdk/compare/9d9bedd051c313cf0f4552c6486c3f43bdaa81b9...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.

Loading

@openjdk openjdk bot added the ready label Feb 23, 2021
@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Feb 23, 2021

/reviewers 2

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 23, 2021

@AlanBateman
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Loading

@openjdk openjdk bot removed the ready label Feb 23, 2021
@MBaesken MBaesken changed the title JDK-8262199: TOCTOU in jli args.c JDK-8262199: issue in jli args.c Feb 23, 2021
} else {
if (st.st_size > MAX_ARGF_SIZE) {
JLI_ReportMessage(CFG_ERROR10, MAX_ARGF_SIZE);
reportAndExit(NULL, NULL);
Copy link
Contributor

@RealCLanger RealCLanger Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be just one statement,
reportAndExit(CFG_ERROR10, MAX_ARGF_SIZE);
or?

Loading

Copy link
Member Author

@MBaesken MBaesken Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that did not work because it does not fit the param types of reportAndExit but I can simplify it otherwise.

Loading

Copy link
Contributor

@RealCLanger RealCLanger Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see.
Maybe it's a bit bike-sheddy but as you're using reportAndExit only twice now, wouldn't it be less convoluted if you use

JLI_ReportMessage(...);
exit(1);

inline in both places?

Loading

Copy link
Member Author

@MBaesken MBaesken Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree, see my latest change.

Loading

@@ -354,42 +354,38 @@ static JLI_List readArgFile(FILE *file) {
return rv;
}

static void reportAndExit(const char* fmt, const char* arg) {
if (fmt != NULL) JLI_ReportMessage(fmt, arg);
Copy link
Contributor

@RealCLanger RealCLanger Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the if (fmt != NULL) check wouldn't be necessary here if you fix the other location with reportAndExit(NULL, NULL), I think

Loading

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Thanks for the updates, the latest looks clean to me.

Loading

Copy link
Contributor

@RealCLanger RealCLanger left a comment

Looks good now :)

Loading

@openjdk openjdk bot added the ready label Feb 26, 2021
Copy link
Contributor

@AlanBateman AlanBateman left a comment

This version looks good too.

Loading

@MBaesken
Copy link
Member Author

@MBaesken MBaesken commented Feb 26, 2021

/integrate

Loading

@openjdk openjdk bot closed this Feb 26, 2021
@openjdk openjdk bot added integrated and removed ready labels Feb 26, 2021
@openjdk openjdk bot removed the rfr label Feb 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 26, 2021

@MBaesken Since your change was applied there have been 44 commits pushed to the master branch:

  • 7603278: 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
  • 0a4e710: 8261954: Dependencies: Improve iteration over class hierarchy under context class
  • 722142e: 8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java
  • bcca100: 4710675: JTextArea.setComponentOrientation does not work with correct timing
  • fce5765: 8262433: doclint: reference error in module jdk.incubator.foreign
  • 059ede0: 8262428: doclint warnings in java.xml module
  • 8256517: 8262421: doclint warnings in jdk.compiler module
  • 29c603f: 8262227: Change SystemDictionary::find() to return an InstanceKlass*.
  • 35c0a69: 8262416: ProblemList TestHeapDumpForLargeArray.java due to JDK-8262386
  • 228c285: 8261170: Upgrade to freetype 2.10.4
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/9d9bedd051c313cf0f4552c6486c3f43bdaa81b9...master

Your commit was automatically rebased without conflicts.

Pushed as commit d7efb4c.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants