[8u] RFR(S): 8244548: JDK 8u: sun.misc.Version.jdkUpdateVersion() returns wrong result

[Severin Gehwolf]

Hi Simon,

On Thu, 2020-05-07 at 11:55 -0400, Simon Tooke wrote:
> (disclaimer: I am not a reviewer)

Thanks for the review.

> 
> I believe it's a "good thing" to shrink the reserved1 bitfield, as I 
> suspect the entire struct grows by 4 bytes otherwise.
> 
> 
> Original:
> 
> 644 unsigned int jdk_version; /* Consists of major, minor, micro (n.n.n) 
> */ 1645 /* and build number (xx) */ 1646 unsigned int update_version : 
> 8; /* Update release version (uu) */ 1647 unsigned int 
> special_update_version : 8; /* Special update release version (c)*/ 1648 
> unsigned int reserved1 : 16; 1649 unsigned int reserved2; The proposed 
> patch: 44 unsigned int jdk_version; /* Consists of major, minor, micro 
> (n.n.n) */ 1645 /* and build number (xx) */ 1646 unsigned int 
> update_version : 16; /* Update release version (uu) */ 1647 unsigned int 
> special_update_version : 8; /* Special update release version (c)*/ 1648 
> unsigned int reserved1 : 16; 1649 unsigned int reserved2; Note that the 
> original sum of bitfields is 32 bits, so it will fit into a 4 byte int 
> (which it is surrounded by) The proposed sum of bitfields is 40 bits, 
> which will probably cause a new 4 byte bitfield to be generated under 
> the hood.

Sure. Updated webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8244548/03/

Thanks,
Severin

> -Simon
> 
> 
> On 2020-05-07 11:09 a.m., Severin Gehwolf wrote:
> > Hi,
> > 
> > Please review this OpenJDK 8u fix for an issue where the update version
> > as configured via --with-update-version=XXX might overflow in internal
> > JDK structures and thus, will get reported wrong. In particular, only 1
> > byte is being reserved for the update versions internally. That is, it
> > works fine up to a configured update version of 255 (2^8 - 1). We've
> > passed that in OpenJDK 8u with the 8u262 cycle currently in EA. Hence,
> > we are now seeing this issue.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8244548
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8244548/02/
> > testing: Linux tier 1
> > 
> > The proposed fix is to extend the update_version field in
> > jdk_version_info from 8 to 16 bit and to use 16 bit of the jvm_version
> > integer in jvm_version_info. Thus, the new upper bound for update
> > version number is 2^16-1 => 65535 which should be sufficient. I don't
> > think OpenJDK 8u will live that long ;-)
> > 
> > jvm_version_info.jvm_version currently holds this quadruplet:
> > 
> > Most significant 8 bits => major version, followed by 8 bits => minor
> > version, followed by 8 bits => micro version, followed by 8 bits =>
> > build version. Note that JVM minor version represents the update
> > version as passed in via configure and the micro version is currently
> > not used (always 0). See vm_version.cpp lines 100-102 where only major,
> > minor and build number are ever been set. Knowing this, we can still
> > preserve the same behavior after patch by defining JVM_VERSION_MICRO to
> > 0 for any version.
> > 
> > For jdk_version_info the fix is simpler, since the update_version is a
> > separate field for which I've extended it to 16 bit. Andrew Brygin
> > suggested to reduce the reserved1 field from currently 16 bit to 8 bit
> > since we are extending update_version by 8 bits, thus making the whole
> > structure grow. I'm not sure reducing reserved1 to 8 bits so as to not
> > grow the structure would be necessary, but I'd be happy to do so if
> > there is such consensus.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> >