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

8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) #7829

Closed
wants to merge 2 commits into from

Conversation

backwaterred
Copy link
Contributor

@backwaterred backwaterred commented Mar 15, 2022

As described in the linked issue, NullClassBytesTest fails due an OutOfMemoryError produced on AIX when the test calls defineClass with a byte array of size of 0. The native implementation of defineClass then calls malloc with a size of 0. On AIX malloc(0) returns NULL, while on other platforms it return a valid address. When NULL is produced by malloc for this reason, ClassLoader.c incorrectly interprets this as a failure due to a lack of memory.

This PR modifies ClassLoader.c to produce an OutOfMemoryError only when errno == ENOMEM and to produce a ClassFormatError with the message "ClassLoader internal allocation failure" in all other cases (in which malloc returns NULL). [edit: The above no longer describes the PR's proposed fix. See discussion below]

In addition, I performed some minor tidy-up work in ClassLoader.c by changing instances of return 0 to return NULL, and if (some_ptr == 0) to if (some_ptr == NULL). This was done to improve the clarity of the code in ClassLoader.c, but didn't feel worthy of opening a separate issue.

Alternatives

It would be possible to address this failure by modifying the test to accept the OutOfMemoryError on AIX. I thought it was a better solution to modify ClassLoader.c to produce an OutOfMemoryError only when the system is actually out of memory.

Testing

This change has been tested on AIX and Linux/x86.


Progress

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

Issue

  • JDK-8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7829

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 15, 2022

👋 Welcome back backwaterred! 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 openjdk bot added the rfr label Mar 15, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2022

@backwaterred The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-runtime

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

@openjdk openjdk bot added hotspot-runtime core-libs labels Mar 15, 2022
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 15, 2022

Webrevs

Copy link
Member

@tstuefe tstuefe left a comment

Hi Tyler,

The way we solve this usually is by homogenizing malloc behavior across all platforms with if (len == 0) len=1;, see e.g.

// On malloc(0), implementators of malloc(3) have the choice to return either
// NULL or a unique non-NULL pointer. To unify libc behavior across our platforms
// we chose the latter.
size = MAX2((size_t)1, size);

I suggest you do that here too since it would be a far less intrusive change. If you don't want to fix up all three malloc call sites, just reroute them to a local malloc stub (my_malloc()) that corrects length and calls the real malloc. If you want, you can #ifdef _AIX that length correction too, e.g.:

static void* my_malloc(size_t l) {
#ifdef  _AIX
if (l == 0) l = 1;
#endif
return malloc(l);
}

That would be a far smaller change, and it would not introduce a new behavioral difference (because with your change, AIX now throws ClassFormatError where other platforms don't).

Side note: nothing against changing 0 to NULL, but please in a separate cleanup patch.

Cheers, Thomas

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Mar 16, 2022

Btw, which malloc call was the problematic exactly? Cannot be the one in getUTF, since that one already adds len + 1 and never gets called with a zero length anyway.

@backwaterred
Copy link
Contributor Author

@backwaterred backwaterred commented Mar 16, 2022

Btw, which malloc call was the problematic exactly? Cannot be the one in getUTF, since that one already adds len + 1 and never gets called with a zero length anyway.

Oh, good point. I guess it gets lost in the noise of my other changes.

The lines causing the issue was these ones:

body = (jbyte *)malloc(length);
if (body == 0) {
JNU_ThrowOutOfMemoryError(env, 0);
return 0;
}

@backwaterred
Copy link
Contributor Author

@backwaterred backwaterred commented Mar 16, 2022

The way we solve this usually is by homogenizing malloc behavior across all platforms with if (len == 0) len=1;

Interesting! That idea didn't occur to me until after I submitted the PR. I'm happy to test that out and see how it works.

@backwaterred
Copy link
Contributor Author

@backwaterred backwaterred commented Mar 16, 2022

Thanks @tstuefe! Your suggestion lead to a better change, so I modified the PR.

  • ClassLoader.c no longer has any reason to throw a ClassFormatError, so that logic is removed.
  • The test no longer needs to recognize a new error message, so that is changed back as well.
  • I also alphabetized the header files, because that is the way I am :-)

Note: I couldn't find an implementation of MAX2 in a C-friendly 'header.h' file, so I just used the ternary operator in the two places I needed it.

@backwaterred backwaterred marked this pull request as draft Mar 16, 2022
@backwaterred
Copy link
Contributor Author

@backwaterred backwaterred commented Mar 16, 2022

Side note: nothing against changing 0 to NULL, but please in a separate cleanup patch.

I just saw this. I will separate the cleanup into a separate patch.

@openjdk openjdk bot removed the rfr label Mar 16, 2022
src/java.base/share/native/libjava/ClassLoader.c Outdated Show resolved Hide resolved
src/java.base/share/native/libjava/ClassLoader.c Outdated Show resolved Hide resolved
- Changes malloc(0) call to malloc(1) on AIX.
@backwaterred backwaterred marked this pull request as ready for review Mar 16, 2022
@openjdk openjdk bot added the rfr label Mar 16, 2022
@backwaterred
Copy link
Contributor Author

@backwaterred backwaterred commented Mar 16, 2022

The requested changes have been made, and a better, leaner PR results.

In addition to the changes mentioned above, I also incorporated Thomas' suggestion to use #ifdef _AIX to ensure the change only happens on AIX where it's needed. I thought I'd skip the call to 'my_malloc' since the change is only required in two places.

Copy link
Member

@tstuefe tstuefe left a comment

This is much better, thanks.

Cheers, Thomas

@openjdk
Copy link

@openjdk openjdk bot commented Mar 17, 2022

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

8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix)

Reviewed-by: stuefe, rriggs, dholmes

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

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@tstuefe, @RogerRiggs, @dholmes-ora) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Mar 17, 2022
@tstuefe
Copy link
Member

@tstuefe tstuefe commented Mar 17, 2022

p.s. this issue puts the finger on a sore point. We have corrected mallocs in the JDK in a number of places, e.g. in libverify:

(

/* On AIX malloc(0) and calloc(0, ...) return a NULL pointer, which is legal,
* but the code here does not handles it. So we wrap the methods and return non-NULL
* pointers even if we allocate 0 bytes.
*/
#ifdef _AIX
static int aix_dummy;
static void* aix_malloc(size_t len) {
if (len == 0) {
return &aix_dummy;
}
return malloc(len);
}
)

(which is also strictly speaking incorrect since the spec says to return a non-unique pointer).

More generally, we have with AIX the problem that APIs have different behavior than the more usual nixes or are missing, and we need a porting layer to provide missing or change different APIs. Beside AIX-ish malloc, one example is dladdr, which is missing.

In hotspot, we have os/aix, so we are fine. See os/aix/hotspot/os/aix/porting_aix.cpp. In the JDK I never found a good place for a porting layer, since the different JDK binaries don't have a common layer. So we have multiple versions of aix malloc and dladdr sprinkled across the libraries, which I always found embarrassing. (outside hotspot we implement dladdr at least in java.desktop/aix/native/libawt/porting_aix.c and java.base/aix/native/libjli/java_md_aix.c).

If you find a way to commonize that code across JDK libraries, that would be cool. I even thought about providing a porting library completely independent from the OpenJDK itself, more like a system library. We did this for our internal iSeries port but the logistics were annoying, so we did not do it for AIX. But you at IBM may have a better idea.

Cheers, Thomas

@backwaterred backwaterred changed the title 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) Mar 17, 2022
- Reword to avoid double use of malloc(X)
- Remove bug id
@backwaterred
Copy link
Contributor Author

@backwaterred backwaterred commented Mar 17, 2022

/integrate

@openjdk openjdk bot added the sponsor label Mar 17, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Mar 17, 2022

@backwaterred
Your change (at version f0e2800) is now ready to be sponsored by a Committer.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Update looks good. Sorry for the AIX_ONLY misdirect.

Thanks,
David

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Mar 18, 2022

/sponsor

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Mar 18, 2022

Update looks good. Sorry for the AIX_ONLY misdirect.

Thanks, David

It would be real nice to have the same set of macros in JDK too, though.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2022

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Mar 18, 2022
@openjdk openjdk bot closed this Mar 18, 2022
@openjdk openjdk bot removed ready rfr sponsor labels Mar 18, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2022

@tstuefe @backwaterred Pushed as commit cab4ff6.

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

@@ -99,7 +99,12 @@ Java_java_lang_ClassLoader_defineClass1(JNIEnv *env,
return 0;
}

// On AIX malloc(0) returns NULL which looks like an out-of-memory condition; so adjust it to malloc(1)
#ifdef _AIX
body = (jbyte *)malloc(length == 0 ? 1 : length);
Copy link
Contributor

@AlanBateman AlanBateman Mar 18, 2022

Choose a reason for hiding this comment

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

Can we use identification in the ifdef/else/endif block to make it a bit more readable. Also can you trim down the comment or split it over two lines to avoid the really long line (it makes it a bit easier for future side-by-side reviews).

Copy link
Contributor Author

@backwaterred backwaterred Mar 18, 2022

Choose a reason for hiding this comment

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

I can split down the comment if you prefer. It might be appropriate to do in the as-yet-unmerged cleanup PR I have open for the same file.

I am not sure what you mean by 'use identification'. Can you clarify?

Choose a reason for hiding this comment

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

I think it’s a typo of “indentation”, e.g.:

Suggested change
body = (jbyte *)malloc(length == 0 ? 1 : length);
body = (jbyte *)malloc(length == 0 ? 1 : length);

Copy link
Contributor Author

@backwaterred backwaterred Mar 18, 2022

Choose a reason for hiding this comment

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

Ah, that does make sense. Please request these changes here and I am happy to make them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs hotspot-runtime integrated
6 participants