RFR: 8178491: -Xss and -XX:ThreadStackSize argument parsing truncates bits
stefan.karlsson at oracle.com
Wed Jun 28 14:05:20 UTC 2017
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