JDK-5044412 : (reflect) if setAccessible(true) has succeeded, allow final fields to be set
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.lang:reflect
  • Affected Version: 5.0
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2004-05-10
  • Updated: 2017-05-16
  • Resolved: 2004-05-12
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
5.0 b51Fixed
Related Reports
Duplicate :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Description
Date: Thu, 06 May 2004 13:50:58 -0400 
From: <###@###.###> 


Background: 
 
JSR133 requires that all fields of classes that are specified to be 
immutable must be declared as "final". This solves a long-standing 
issue in the memory semantics of Java that was not addressed in the 
original JLS. The presense of "final" in such cases is more than a 
decoration. It causes the compiler and hardware to not reorder 
critical instructions in a way that could cause one thread to "catch" 
an immutable object in the course of construction, and thus see its 
pre-construction (0/null) value instead of its actual final 
value. This is not just a theoretical possibility; without the use of 
"final", this can and does occur regularly on multiprocessors. A 
further discussion with examples of how this may induce security holes 
may be found at 
http://www.cs.umd.edu/users/pugh/java/memoryModel/jsr-133-faq.html#finalWrong 
 
As part of JSR133, the expert group surveyed all classes in java.lang 
and java.util, to find those that required field declarations be 
marked as "final" to meet their specs. This was then done as bug 
4998264. 
 
While this is all fine, the JSR133 group did not address (or 
anticipate the consequences of) a known interaction between "final" 
and code that does "manual" deserialization, such as the XML 
serialization in appServer: It is impossible to reflectively assign a 
value to a final field. Thus, appServer broke. This might be 
indicative of other similar code out there also being broken. 
 
While it might be deal with this by undoing some of the "finals" in 
4998264, this would be a crummy alternative in two senses: (1) It 
merely delays the day of reckoning for dealing with the underlying 
problem (2) People writing multithreaded code will be relying on 
immutability guarantees that are not actually supported, so will 
encounter extremely difficult to locate bugs. 
 
Proposal:  
  Allow reflective set-field operations on any final field f, 
  ONLY if f.setAccessible(true) succeeds. 
 
Advantages: 
  1. appServer will actually work. (Not just sort-of work, as true even 
     when using 1.4.x). Any other similar code out there that bothers 
     to do explicit setAccessible calls before setting fields will 
     similarly work.
  2. In some limited circumstances, this technique can be used in 
     a serializable class's custom readObject method to initialize 
     final fields upon deserialization directly (rather than through 
     defaultReadObject).  Note that because of the permission 
     requirement, this only applies to highly trusted code; it is not 
     a general solution. 
  3. No API change. It only requires addition of one new sentence in  
     the javadocs to describe the case when setting final is allowed. 
  4. By requiring the setAccessible check EVEN if the field is 
     public, there is essentially no possibility of accidental misuse. 
  5. If you have permission to suppress all access checks, then you can, 
     among other things change the security manager, so there is 
     no additional security risk here. 
  6. Bill Pugh and I can/will adjust the JSR133 spec to add the appropriate 
     wording about resulting multithreaded semantics. 
  7. There is almost no impact on plans to further optimize final fields 
     in the future inside hotspot. 
 
  8. And mainly: I think this is the simplest solution that actually works. 
 
Disadvantages: 
  1. While the changes are straightforward, there are more lines of 
     them than anyone wants to see at this point. 
  2. If people use this in inappropriate contexts (i.e., in situations 
     not related to object reconstruction or deserialization)  
     their programs will make no sense. 

Comments
CONVERTED DATA BugTraq+ Release Management Values COMMIT TO FIX: tiger-beta2 FIXED IN: tiger-beta2 INTEGRATED IN: tiger-b51 tiger-beta2
2004-06-14

SUGGESTED FIX Date: Thu, 06 May 2004 12:41:34 -0400 From: <###@###.###> Being appropriately terrified about last minute changes, I factored this a little differently than planned. Instead of adding code to the existing UnsafeXFieldAccessors, I made a new parallel set of UnsafeQualifiedXFieldAccessors to handle the new cases. This at first might seem excessive/crazy, but has the following advantages: * The UnsafeXFieldAccessors are completely untouched, so there are no questions about performance or functionality differences. * The new Qualified versions are easy to manually check for correctness: they add only the isReadOnly args/checks, and use the Volatile unsafe calls instead of the plain ones. You can review these simply by looking at diffs from plain versions, leaving practically no chance of error. (The "diffs/qdiffs" file in attached zip includes this set of diffs.) Even though doing it the other way was straightforward, I found myself glazing over while trying to make sure all the final/volatile cases correctly paralleled the others. (There are 92 of these calls, each of which needed the associated variant. Again, this is easy, but also too easy to get one of them wrong and never see it.) And I also worried about possible performance issues in the usual non-final case due to making methods bigger. Even though I didn't do so, the "isFinal" checks in the plain accessors can go away since they should never be used. I didn't do this, just out of paranoia -- in case these classes are ever constructed for some other reason. (Surely they aren't, but I figure paranoia is right stance.) The other changes are 1. Extended dispatching logic in UnsafeFieldAccessorFactory to create the appropriate kind of Accessor. (This is now the only place you find a big ugly conditional.) 2. addition of an optional (i.e. overloaded) argument to ReflectionFactory.newFieldAccessor to propagate override, and removing disabling clause for volatile case since it is now covered. 3. Providing the override arg to the call from Field.java 4. Adding two sentences to item on finals in javadoc for Field.set method changed and extended the jtreg test bug4250960.java that was in I changed and extended the jtreg test bug4250960.java that was in test/java/lang/reflect/Field so that it checks for new access rules. This is only a light test; I'll further extend later. The JCK folks might have tests that similarly need changing.
2004-06-11

EVALUATION This should be considered for tiger-beta2. -- iag@sfbay 2004-05-10
2004-05-10