8296496: Overzealous check in sizecalc.h prevents large memory allocation #11030

azuev-java · 2022-11-07T22:04:55Z

Removed the additional multiplication overflow detection.
Instead cast all the parameters to type_t just the way they are treated in the existing size check macro.
This way there is no possibility to accidentally provide parameters that will pass the size check macro while being cast to size_t there but then due to the missing cast cause the wrong size passed the actual allocation function.
Since this checking macro was used in couple of different places all of them needs to be updated in the similar way.

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-8296496: Overzealous check in sizecalc.h prevents large memory allocation

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Alexander Zvegintsev (@azvegint - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11030

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

Using diff file

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

Instead cast the parameters to type_t just the way they are treated in the existing size check macro. Since this checking maccro was used in couple of different places all of them needs to be updated.

bridgekeeper · 2022-11-07T22:06:33Z

👋 Welcome back kizune! 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 · 2022-11-07T22:09:27Z

@azuev-java 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 · 2022-11-07T22:13:48Z

Webrevs

victordyakov · 2022-11-07T23:14:03Z

@aivanov-jdk @azvegint @prrace please review

aivanov-jdk · 2022-11-08T13:50:25Z

src/java.desktop/share/native/common/awt/utility/sizecalc.h


 #define SAFE_SIZE_NEW_ARRAY2(type, n, m) \
    (IS_SAFE_SIZE_MUL((m), (n)) && IS_SAFE_SIZE_MUL(sizeof(type), (n) * (m)) ? \
-     (new type[(n) * (m)]) : throw std::bad_alloc())
+     (new type[(size_t)((n) * (m))]) : throw std::bad_alloc())


Suggested change

(new type[(size_t)((n) * (m))]) : throw std::bad_alloc())

(new type[(size_t)(n) * (size_t)(m)]) : throw std::bad_alloc())

Each parameter must be cast as in SAFE_SIZE_ARRAY_ALLOC.

Each parameter must be cast as in SAFE_SIZE_ARRAY_ALLOC.

If we do that then logic might be broken since we checking for the limits against the (size_t)(m * n) but performing call with ((size_t)(m) * (size_t)(n)) which might add potential point of failure. I prefer to do the conversions in the same way we do them in checks.

It's what you just did for SAFE_SIZE_ARRAY_ALLOC:

#define SAFE_SIZE_ARRAY_ALLOC(func, m, n) \ - (IS_SAFE_SIZE_MUL((m), (n)) ? ((func)((m) * (n))) : FAILURE_RESULT) + (IS_SAFE_SIZE_MUL((m), (n)) ? ((func)((size_t)(m) * (size_t)(n))) : FAILURE_RESULT)

You cast both m and n.

This code basically does the same. If you don't cast each parameter, the result may be different, since there are cases where ((size_t)(m) * (size_t)(n)) is not equal to (size_t)((m) * (n)).

Ok, i see what you mean. Ok, that might work since we are already checking SAFE_SIZE_MUL((m), (n)) in the line above.

aivanov-jdk · 2022-11-08T13:54:31Z

src/java.desktop/share/native/common/awt/utility/sizecalc.h

@@ -116,7 +112,7 @@
 *    // Use the allocated memory...
 */
 #define SAFE_SIZE_STRUCT_ALLOC(func, a, m, n) \
-    (IS_SAFE_STRUCT_SIZE((a), (m), (n)) ? ((func)((a) + (m) * (n))) : FAILURE_RESULT)
+    (IS_SAFE_STRUCT_SIZE((a), (m), (n)) ? ((func)((a) + (size_t)(m) * (size_t)(n))) : FAILURE_RESULT)


Suggested change

(IS_SAFE_STRUCT_SIZE((a), (m), (n)) ? ((func)((a) + (size_t)(m) * (size_t)(n))) : FAILURE_RESULT)

(IS_SAFE_STRUCT_SIZE((a), (m), (n)) ? ((func)((size_t)(a) + (size_t)(m) * (size_t)(n))) : FAILURE_RESULT)

To be safe, a should also be cast.

And IS_SAFE_STRUCT_SIZE should also be updated to pass (size_t)(m) * (size_t)(n) to IS_SAFE_SIZE_ADD instead of (m) * (n).

To be safe, a should also be cast.

And IS_SAFE_STRUCT_SIZE should also be updated to pass (size_t)(m) * (size_t)(n) to IS_SAFE_SIZE_ADD instead of (m) * (n).

Sounds reasonable.

mlbridge · 2022-11-08T22:19:57Z

Mailing list message from Aleksei Ivanov on client-libs-dev:

Hi Patrick,

It is the reason why it should be updated. The checks in
IS_SAFE_STRUCT_SIZE are performed with the cast: (size_t)(a). If the
cast is omitted here, it may yield a different result.

What if ?a? is a signed integer with negative value?

Regards,
Alexey

On 08/11/2022 15:25, Patrick Chen wrote:

openjdk · 2022-11-09T11:35:44Z

@azuev-java 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:

8296496: Overzealous check in sizecalc.h prevents large memory allocation

Reviewed-by: aivanov, 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 124 new commits pushed to the master branch:

e1badb7: 8295871: G1: Use different explicit claim marks for CLDs
9ef7852: 8290714: Make com.sun.jndi.dns.DnsClient virtual threads friendly
d6468be: 8293886: The abstract keyword can be removed in AESCipher
54c986e: 8296715: CLDR v42 update for tzdata 2022f
4a68210: 6972078: Can not select single directory with GTKLookAndFeel
4465361: 8295948: Support for Zicbop/prefetch instructions on RISC-V
f2acdfd: 8296638: RISC-V: NegVI node emits wrong code when vector element basic type is T_BYTE/T_SHORT
bfc5816: 8295475: Move non-resource allocation strategies out of ResourceObj
e802b12: 8296196: Class.getEnumConstants() throws undocumented ClassCastException and NullPointerException
78a08a0: 8295430: Use cmsDoTransformLineStride instead of cmsDoTransform in the loop
... and 114 more: https://git.openjdk.org/jdk/compare/590de37bd703bdae56e8b41c84f5fca5e5a00811...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.

mlbridge · 2022-11-09T19:57:00Z

Mailing list message from Patrick Chen on client-libs-dev:

But you forgot that (IS_SAFE_STRUCT_SIZE((a), (m), (n)) ? ((func)((a) +
(size_t)(m) * (size_t)(n))) : FAILURE_RESULT) is not the same equivalence
to (IS_SAFE_STRUCT_SIZE((a), (m), (n)) ? ((func)((size_t)(a) +
(size_t)(m) * (size_t)(n))) : FAILURE_RESULT)
because of the ((func)((size_t)(a) + (size_t)(m) * (size_t)(n))

Le mar. 8 nov. 2022 ? 14:59, Alexey Ivanov <aivanov at openjdk.org> a ?crit :

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/client-libs-dev/attachments/20221108/b0a36934/attachment.htm>

azuev-java · 2022-11-10T22:41:37Z

/integrate

openjdk · 2022-11-10T22:43:10Z

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

27527b4: 8296612: CertAttrSet is useless
6b456f7: 8262901: [macos_aarch64] NativeCallTest expected:<-3.8194101E18> but was:<3.02668882E10>
e1badb7: 8295871: G1: Use different explicit claim marks for CLDs
9ef7852: 8290714: Make com.sun.jndi.dns.DnsClient virtual threads friendly
d6468be: 8293886: The abstract keyword can be removed in AESCipher
54c986e: 8296715: CLDR v42 update for tzdata 2022f
4a68210: 6972078: Can not select single directory with GTKLookAndFeel
4465361: 8295948: Support for Zicbop/prefetch instructions on RISC-V
f2acdfd: 8296638: RISC-V: NegVI node emits wrong code when vector element basic type is T_BYTE/T_SHORT
bfc5816: 8295475: Move non-resource allocation strategies out of ResourceObj
... and 116 more: https://git.openjdk.org/jdk/compare/590de37bd703bdae56e8b41c84f5fca5e5a00811...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-10T22:43:31Z

@azuev-java Pushed as commit 84e1224.

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

mrserb · 2022-11-14T19:20:25Z

src/java.desktop/share/native/common/awt/utility/sizecalc.h

@@ -92,19 +88,19 @@
 * IS_SAFE_... macros to check if the calculations are safe.
 */
 #define SAFE_SIZE_NEW_ARRAY(type, n) \
-    (IS_SAFE_SIZE_MUL(sizeof(type), (n)) ? (new type[(n)]) : throw std::bad_alloc())
+    (IS_SAFE_SIZE_MUL(sizeof(type), (n)) ? (new type[(size_t)(n)]) : throw std::bad_alloc())

 #define SAFE_SIZE_NEW_ARRAY2(type, n, m) \
    (IS_SAFE_SIZE_MUL((m), (n)) && IS_SAFE_SIZE_MUL(sizeof(type), (n) * (m)) ? \


Why we do not cast it here: (n) * (m)?

Filed https://bugs.openjdk.org/browse/JDK-8296957

You're right, it must be cast here too, I have missed it.

Would you mind submitting a bug?

Removed the additional multiplication overflow detection.

3a3e776

Instead cast the parameters to type_t just the way they are treated in the existing size check macro. Since this checking maccro was used in couple of different places all of them needs to be updated.

openjdk bot added the rfr Pull request is ready for review label Nov 7, 2022

openjdk bot added the client client-libs-dev@openjdk.org label Nov 7, 2022

aivanov-jdk suggested changes Nov 8, 2022

View reviewed changes

azuev-java added 2 commits November 8, 2022 15:19

Streamlining structure allocation

ee223fc

Fixed casting in SAFE_SIZE_NEW_ARRAY2

9673bac

aivanov-jdk approved these changes Nov 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 9, 2022

azvegint approved these changes Nov 10, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 10, 2022

openjdk bot closed this Nov 10, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 10, 2022

mrserb reviewed Nov 14, 2022

View reviewed changes

	(new type[(size_t)((n) * (m))]) : throw std::bad_alloc())
	(new type[(size_t)(n) * (size_t)(m)]) : throw std::bad_alloc())

	(IS_SAFE_STRUCT_SIZE((a), (m), (n)) ? ((func)((a) + (size_t)(m) * (size_t)(n))) : FAILURE_RESULT)
	(IS_SAFE_STRUCT_SIZE((a), (m), (n)) ? ((func)((size_t)(a) + (size_t)(m) * (size_t)(n))) : FAILURE_RESULT)

8296496: Overzealous check in sizecalc.h prevents large memory allocation #11030

8296496: Overzealous check in sizecalc.h prevents large memory allocation #11030

Uh oh!

Conversation

azuev-java commented Nov 7, 2022 • 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 Nov 7, 2022

Uh oh!

openjdk bot commented Nov 7, 2022

Uh oh!

mlbridge bot commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

victordyakov commented Nov 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Nov 8, 2022

Uh oh!

openjdk bot commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Nov 9, 2022

Uh oh!

azuev-java commented Nov 10, 2022

Uh oh!

openjdk bot commented Nov 10, 2022

Uh oh!

openjdk bot commented Nov 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

azuev-java commented Nov 7, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 7, 2022 •

edited

Loading

openjdk bot commented Nov 9, 2022 •

edited

Loading