RFR 8171132: Improve class reading of invalid or out-of-range ConstantValue attributes

[Maurizio Cimadamore]

Overall the patch looks good, but I have a couple of comments:

This line:

if (var.type.tsym == syms.stringType.tsym) {

should probably be replaced with:

Assert.check(var.type.tsym == syms.stringType.tsym)


As I'm not sure we even want to proceed if the variable type is other 
than String.

On a related note, your convertType routine seems weak - it does all the 
required conversions - but without actively checking ranges and throwing 
errors if something bad is detected (e.g. a 'byte' field, whose constant 
value is 1000). Instead, I believe your code just truncates the part in 
excess, which, while acceptable (after all, the creator of such a bad 
classfile gets what he/she deserves) - could lead to some painful 
debugging; so an eager check/error would be preferrable, I think.

Note that we already have code like this:

case BYTE:
                 if (Byte.MIN_VALUE <= value && value <= Byte.MAX_VALUE)
                     return true;
                 break;
             case CHAR:
                 if (Character.MIN_VALUE <= value && value <= 
Character.MAX_VALUE)
                     return true;
                 break;
             case SHORT:
                 if (Short.MIN_VALUE <= value && value <= Short.MAX_VALUE)
                     return true;
                 break;
             case INT:
                 return true;

In Types.isAssignable. I suggest we refactor that code into a standalone 
routine - either in Types or even in TypeTag - something like

boolean checkRange(int value)

And then we use that from both Types.isAssignable and also your new 
convertType routine. In you go down this path, I think the test should 
be expanded to cover this case too.

What do you think?

Maurizio

On 13/12/16 03:29, Liam Miller-Cushon wrote:
> Hello,
>
> As discussed in:
> http://mail.openjdk.java.net/pipermail/compiler-dev/2016-November/010498.html
>
> javac does not currently check for invalid or out-of-range 
> ConstantValue attributes.
>
> The proposed change is to emit an error for class files where a 
> ConstantValue attribute disagrees with the field type, and to narrow 
> any out-of-range values for bool, short, byte or char constant fields 
> to within the expected range.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8171132
> Webrev: http://cr.openjdk.java.net/~cushon/8171132/webrev.00 
> <http://cr.openjdk.java.net/%7Ecushon/8171132/webrev.00/>
>
> Any feedback is welcome.
>
> Thanks,
> Liam

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20161214/33407165/attachment.html>