Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Fri, 18 Mar 2022 07:43:21 GMT, Alan Bateman wrote: >> Tyler Steele has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Improve comment >> >> - Reword to avoid double use of malloc(X) >> - Remove bug id > > src/java.base/share/native/libjava/ClassLoader.c line 104: > >> 102: // On AIX malloc(0) returns NULL which looks like an out-of-memory >> condition; so adjust it to malloc(1) >> 103: #ifdef _AIX >> 104: body = (jbyte *)malloc(length == 0 ? 1 : length); > > 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). 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? - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Fri, 18 Mar 2022 15:25:20 GMT, ExE Boss wrote: >> 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? > > I think it’s a typo of “indentation”, e.g.: > Suggestion: > > body = (jbyte *)malloc(length == 0 ? 1 : length); Ah, that does make sense. Please request these changes [here](https://github.com/openjdk/jdk/pull/7846) and I am happy to make them. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Fri, 18 Mar 2022 15:22:48 GMT, Tyler Steele wrote: >> src/java.base/share/native/libjava/ClassLoader.c line 104: >> >>> 102: // On AIX malloc(0) returns NULL which looks like an out-of-memory >>> condition; so adjust it to malloc(1) >>> 103: #ifdef _AIX >>> 104: body = (jbyte *)malloc(length == 0 ? 1 : length); >> >> 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). > > 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? I think it’s a typo of “indentation”, e.g.: Suggestion: body = (jbyte *)malloc(length == 0 ? 1 : length); - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Thu, 17 Mar 2022 16:23:14 GMT, Tyler Steele wrote: >> 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. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > Improve comment > > - Reword to avoid double use of malloc(X) > - Remove bug id src/java.base/share/native/libjava/ClassLoader.c line 104: > 102: // On AIX malloc(0) returns NULL which looks like an out-of-memory > condition; so adjust it to malloc(1) > 103: #ifdef _AIX > 104: body = (jbyte *)malloc(length == 0 ? 1 : length); 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). - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Fri, 18 Mar 2022 02:15:36 GMT, David Holmes wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Thu, 17 Mar 2022 16:23:14 GMT, Tyler Steele wrote: >> 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. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > Improve comment > > - Reword to avoid double use of malloc(X) > - Remove bug id Update looks good. Sorry for the AIX_ONLY misdirect. Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Thu, 17 Mar 2022 16:23:14 GMT, Tyler Steele wrote: >> 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. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > Improve comment > > - Reword to avoid double use of malloc(X) > - Remove bug id Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
> 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. Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: Improve comment - Reword to avoid double use of malloc(X) - Remove bug id - Changes: - all: https://git.openjdk.java.net/jdk/pull/7829/files - new: https://git.openjdk.java.net/jdk/pull/7829/files/f05c8d85..f0e2800e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7829=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7829=02-03 Stats: 6 lines in 1 file changed: 0 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7829.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7829/head:pull/7829 PR: https://git.openjdk.java.net/jdk/pull/7829
