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

8266171: -Warray-bounds happens in imageioJPEG.c #3788

Closed
wants to merge 2 commits into from

Conversation

@YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Apr 29, 2021

We can see following compiler warnings in imageioJPEG.c on GCC 11.

/home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c: In function 'Java_com_sun_imageio_plugins_jpeg_JPEGImageWriter_initJPEGImageWriter':
/home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c:673:23: warning: array subscript 'struct jpeg_decompress_struct[0]' is partly outside array bounds of 'unsigned char[520]' [-Warray-bounds]
  673 |             free(dinfo->src);
      |                  ~~~~~^~~~~
/home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c:2538:9: note: referencing an object of size 520 allocated by 'malloc'
 2538 |         malloc(sizeof(struct jpeg_compress_struct));
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c:674:24: warning: array subscript 'struct jpeg_decompress_struct[0]' is partly outside array bounds of 'unsigned char[520]' [-Warray-bounds]
  674 |             dinfo->src = NULL;
      |                        ^
/home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c:2538:9: note: referencing an object of size 520 allocated by 'malloc'
 2538 |         malloc(sizeof(struct jpeg_compress_struct));
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

GCC reports disposing resource for "j_decompress_ptr" in L673-634, however GCC confused it is allocated for "j_compress_ptr".
The code is correct, so we can avoid this warning with pragma.


Progress

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

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3788/head:pull/3788
$ git checkout pull/3788

Update a local copy of the PR:
$ git checkout pull/3788
$ git pull https://git.openjdk.java.net/jdk pull/3788/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3788

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3788.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 29, 2021

👋 Welcome back ysuenaga! 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
Copy link

@openjdk openjdk bot commented Apr 29, 2021

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

  • 2d

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 2d label Apr 29, 2021
@YaSuenag YaSuenag marked this pull request as ready for review Apr 29, 2021
@openjdk openjdk bot added the rfr label Apr 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 29, 2021

Webrevs

Loading

@@ -670,8 +670,17 @@ static void imageio_dispose(j_common_ptr info) {
info->err = NULL;
if (info->is_decompressor) {
j_decompress_ptr dinfo = (j_decompress_ptr) info;
#ifdef __GNUC__
Copy link
Contributor

@prrace prrace Apr 29, 2021

Choose a reason for hiding this comment

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

I know how these structs are defined but I am not sure how gcc can decide anything like this.
I'd almost worry if it were true that we had the other type despite what the flag said except I can't
imagine gcc is doing even any static analysis of the code calling sequence and you may even need a dynamic analysis for this.

Have you submitted a gcc bug ?
Why is it only complaining in this branch ?
Have you considered disabling the warning in the make files - with broader scope of course - but a simpler change ?

Is 520 bytes the actual size of the compress struct ? And even then I am not sure I know what the compiler message means. so long as we have the right starting address free will only free what was allocated ... surely ...

Loading

Copy link
Member Author

@YaSuenag YaSuenag Apr 30, 2021

Choose a reason for hiding this comment

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

I also think GCC does not do any static analysis of the code calling. And also the code uses struct like a C++ class, so I guess GCC reports incorrect warnings at this point.
Many -Warray-bounds related issues are reported in GCC Bugzilla. Especially it does not seem to work when the value is accessed by a pointer https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99474 - it looks alike to me.

WebKit seems to encounter similar issue, they has avoided it with pragma.

I concidered to disable it with a compiler option in makefile of course as you said, but I didn't do so because I heard in other review (I forget the JBS ticket) that we should disable warnings locally because they might be useful in future.

Loading

Copy link
Contributor

@prrace prrace Apr 30, 2021

Choose a reason for hiding this comment

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

In this case the rest of the code here is unlikely to change so I think it is fine to put it in the build and
it will be easier to revert it when gcc is fixed .. and since the warning looks like it is bogus more often
than not wider blocking of it is probably OK anyway. But I am also sure this is the only case.

Loading

Copy link
Member Author

@YaSuenag YaSuenag May 1, 2021

Choose a reason for hiding this comment

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

I disabled -Warray-bounds in Awt2dLibraries.gmk in new commit. Is it ok?

Loading

prrace
prrace approved these changes May 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 4, 2021

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

8266171: -Warray-bounds happens in imageioJPEG.c

Reviewed-by: prr

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

  • 80323b7: 8261169: Upgrade HarfBuzz to the latest 2.8.0
  • 9c4efdd: 8263124: Missed initialization of baselineY in sun.font.StrikeMetrics
  • 4e96b31: 8265989: System property for the native character encoding name
  • 8b37d48: 8255493: Support for pre-generated java.lang.invoke classes in CDS dynamic archive
  • 770dfc1: 8265279: Remove unused RandomGeneratorFactory.all(Class category)
  • ee5bba0: 8265767: compiler/eliminateAutobox/TestIntBoxing.java crashes on arm32 after 8264649 in debug VMs
  • 05e6017: 8265137: java.util.Random suddenly has new public methods nowhere documented
  • aa90df6: 8266187: Memory leak in appendBootClassPath()
  • b651904: 8266438: Compile::remove_useless_nodes does not remove opaque nodes
  • 141cc2f: 8261527: Record page size used for underlying mapping in ReservedSpace
  • ... and 63 more: https://git.openjdk.java.net/jdk/compare/56cde70da271ef5437d5544b3bd9ba26e4148af1...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 May 4, 2021
@YaSuenag
Copy link
Member Author

@YaSuenag YaSuenag commented May 5, 2021

/integrate

Loading

@openjdk openjdk bot closed this May 5, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 5, 2021

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

Your commit was automatically rebased without conflicts.

Pushed as commit b172555.

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

Loading

@YaSuenag YaSuenag deleted the JDK-8266171 branch May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants