RFR: 8178491: -Xss and -XX:ThreadStackSize argument parsing truncates bits
stefan.karlsson at oracle.com
Thu Jun 29 11:33:19 UTC 2017
I got a request form Kim to remove the friend macrology in the test.
Here's the update webrevs:
On 2017-06-28 16:05, Stefan Karlsson wrote:
> Hi all,
> Please review this patch to fix bugs in the -Xss parsing.
> I found this bug while working on an Enhancement to the align functions:
> In arguments.cpp we have the following code in the section that
> converts the Xss value to a ThreadStackSize value:
> round_to((int)long_ThreadStackSize, K) / K) != Flag::SUCCESS)
> This explicit narrowing cast throws away the upper 32 bits of
> long_ThreadStackSize, the resulting int is then promoted to an
> intptr_t used as a argument to round_to. This causes weird error
> messages and behavior. For example, running:
> java -Xss2147483648 -version
> intx ThreadStackSize=18014398507384832 is outside the allowed range [
> 0 ... 9007199254740987 ]
> My initial reaction was that this could be solved by simply fixing the
> cast, but further inspection of the code parsing -Xss and
> ThreadStackSize revealed more issues.
> Changes in this patch:
> 1) Extract the -Xss parsing code into a separate, testable function.
> Unit tests added.
> 2) Extended parse_memory_size and check_memory_size, to take the
> maximum input value. Unit tests added.
> 3) Provide an upper limit for the values allowed in the call to
> parse_memory_size in the -Xss parsing. The upper limit is calculated
> backwards from the the upper limit of the ThreadStackSize.
> 4) The max input value for ThreadStackSize was artificially limited by
> the fact that round_to was used, and that limited values to fit in an
> intptr_t, even though size_t is used when the ThreadStackSize is
> finally expanded into page size aligned bytes. This meant that we had
> an extremly high upper limit, but it wasn't the absolute upper limit.
> I changed this to the absolute upper limit, so that it's easier to
> reason about why this limit was chosen. To make this possible, I
> replaced the usages round_to and round_down, with align_size_up_ and
> align_size_down_ macros. In future patches, when the input parameter
> types of our align functions have been fixed, I'll convert the usages
> of these macros.
> 5) More asserts to check for unintended overflows.
> IMHO, It's a bit silly to allow this high values for -Xss, and we
> could probably limit it to more practical values. I experimented with
> that, but I found that the code and tests became more unintuitive than
> they already are. So, I'd like to leave that as a future enhancement
> if the maintainers of this code allows it.
> Tested with JPRT, TooSmallStackSize.java, and
> TestThreadStackSizes.java. Any suggestions on how to further test this?
More information about the hotspot-runtime-dev