RFR: 8178491: -Xss and -XX:ThreadStackSize argument parsing truncates bits

[Stefan Karlsson]

Hi all,

I got a request form Kim to remove the friend macrology in the test. 
Here's the update webrevs:
  http://cr.openjdk.java.net/~stefank/8178491/webrev.01
  http://cr.openjdk.java.net/~stefank/8178491/webrev.01.delta

StefanK

On 2017-06-28 16:05, Stefan Karlsson wrote:
> Hi all,
>
> Please review this patch to fix bugs in the -Xss parsing.
>
> http://cr.openjdk.java.net/~stefank/8178491/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8178491
>
> I found this bug while working on an Enhancement to the align functions:
> https://bugs.openjdk.java.net/browse/JDK-8178489
>
> 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
>
> Yields:
> 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?
>
> Thanks,
> StefanK