please review fix for 7118546, warnings cleanup in javax.xml.crypto

Stuart Marks stuart.marks at oracle.com
Thu Dec 8 01:38:03 UTC 2011


Webrev is here:

http://cr.openjdk.java.net/~smarks/reviews/7118546/webrev.0/

Notes:

In addition to changes to javax.xml.crypto, this webrev also includes a change 
to a file in javax.script. This package (among others) is included in the same 
build step as javax.xml.crypto, specifically in jdk/make/javax/others/Makefile. 
With this change, this entire build step will become warning-free.

Many of the changes are simply adding @SuppressWarnings("rawtypes") at 
locations where raw types are used in the API. I tried to be careful not to 
change any actual APIs, but please recheck this. (During this warnings effort, 
inadvertent API changes seem to be catching everyone.)

Of particular note are changes in the following files:

  - ExcC14NParameterSpec.java
  - XPathFilter2ParameterSpec.java
  - XPathFilterParameterSpec.java
  - XPathType.java

The constructors in these classes take a raw List or Map, make a defensive 
copy, run through the elements and use instanceof to ensure they're all of the 
right type, create an unmodifiable wrapper and then assign it to a field. The 
current code uses all raw types.

(For the following discussion it probably would be useful to bring up the code 
for the XPathType constructor.)

There were several alternative approaches to getting rid of the warnings in 
this code. Since the code inevitably would have raw types and unchecked casts, 
one approach would simply be to put @SuppressWarnings({"rawtypes","unchecked"}) 
at the top of the constructor and be done with it. However, this obscures up a 
bunch of important details and IMHO covers too much code with @SuppressWarnings.

Another approach would be to do an unchecked cast (with warning suppressed) 
directly from the raw type to the destination generic type such as 
Map<String,String>. The code would then run through the entries to ensure that 
each key and value is indeed a String. This seems odd, and might lead a future 
maintenance programmer to "optimize away" such checks. Of course, these dynamic 
type checks are important for the security of the system.

The approach I ended up taking was to make the defensive copy into a wildcarded 
type such as Map<?,?> which I believe is an accurate type: it's a map, but we 
don't know the actual types of its keys and values. Only after doing all the 
instanceof checks do we do the cast to Map<String,String>. Unfortunately, the 
resulting code is longer and has a proliferation of wildcards, making it appear 
to be quite complex. But I think it actually makes the most sense of the 
alternatives.

If we want to simplify this code, I'd suggest we convert these cases to use the 
enhanced-for loop. I've refrained from doing so since we're avoiding 
refactoring as part of the warnings cleanup exercise.

But let me know what you think.

s'marks



More information about the security-dev mailing list