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

[Severin Gehwolf]

Hi,

On Wed, 2020-05-13 at 16:11 +0100, Andrew Hughes wrote:
> On 12/05/2020 15:14, Severin Gehwolf wrote:
> > Hi Andrew,
> > 
> > Thanks for looking at this.
> > 
> > On Tue, 2020-05-12 at 14:07 +0100, Andrew Haley wrote:
> > > On 5/7/20 4:09 PM, Severin Gehwolf wrote:
> > > > 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.
> > > 
> > > This is tricky. JVM_GetVersionInfo is a function exported by
> > > libjvm.so, and the version is simply encoded as
> > > 
> > > unsigned int Abstract_VM_Version::jvm_version() {
> > >   return ((Abstract_VM_Version::vm_major_version() & 0xFF) << 24) |
> > >          ((Abstract_VM_Version::vm_minor_version() & 0xFF) << 16) |
> > >          (Abstract_VM_Version::vm_build_number() & 0xFF);
> > > }
> > > 
> > > I guess we could argue that this is for JVM-JDK internal use only, and
> > > no-one else cares.
> > > 
> > > Or we could encode it in a different way such that at least we have a
> > > jvm_version that is monotonically increasing, perhaps by (ab)using the
> > > lower 8 bits of the version, the vm_build_number. But I guess I'm
> > > being paranoid, and no tools are going to care about minor versions
> > > anyway,even if they do call JVM_GetVersionInfo.
> > 
> > Yes, this is indeed tricky. The trouble is that
> > Abstract_VM_Version::vm_build_number() actually holds the configured
> > JDK build number, we can't really use it. It's already being used.
> > 
> > (gdb)
> > 99	
> > 100	  _vm_major_version = atoi(vm_major_ver);
> > 101	  _vm_minor_version = atoi(vm_minor_ver);
> > 102	  _vm_build_number  = atoi(vm_build_num);
> > 103	
> > 104	  os::free(vm_version);
> > 105	  _initialized = true;
> > 106	}
> > 107	
> > 108	#if defined(_LP64)
> > (gdb) p _vm_build_number 
> > $1 = 2
> > 
> > The only bits which seem unused are bits 8 through 16 of this unsigned
> > int. So even if JVM_GetVersionInfo *is* being used somewhere in the
> > wild, it *should* continue to work. Hence, the proposal to now also use
> > those unused bits for the minor version.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 
> 
> The April release of OpenJDK 7u has the values:
> 
> Major version: 24, minor version: 5, micro version: 0, build number: 2
> 
> when it should be 24.261-b02. So we already have a situation that could
> be potentially be misinterpreted as an old JDK version, 7u5 b02, using
> the current interpretation of the structure. 8u will see similar issues
> if we make no change in this cycle.

Yes. I believe we need to get this fixed. The earlier in the 262 update
cycle the better.

> As the micro version appears to have always been zero, it seems pretty
> safe to just hard-code this value and instead use all 16 bits of the
> interpreted value for the micro version. In terms of backwards
> compatibility, if a micro version greater than zero is read using the
> old method, this will be an indicator that it needs to be
> re-interpreted as a 16-bit value.

As Andrew Hughes mentioned below, the only way to see the difference
post-patch would be for somebody to use old jvm.h defines *and* use a
supposedly a JDK private API (JVM_GetVersionInfo). It is an exported
symbol, but as Andrew Hughes pointed out, for JDK <=> hotspot
communication.

On the other hand, the chance of somebody using forbidden (access
restricted), yet available API via sun.misc.Version is more likely. Of
which the following are broken (see my latest comment on the bug[1]):

sun.misc.Version.jvmMinorVersion()
sun.misc.Version.jdkUpdateVersion()

Post patch the above are fixed, but somebody using JVM_GetVersionInfo
directly with old jvm.h macros would get for 8u262-b03 (pseudo code):

#define JVM_VERSION_MAJOR(version) ((version & 0xFF000000) >> 24)
#define JVM_VERSION_MINOR(version) ((version & 0x00FF0000) >> 16)
#define JVM_VERSION_MICRO(version) ((version & 0x0000FF00) >> 8)
#define JVM_VERSION_BUILD(version) ((version & 0x000000FF))

jvm_version_info info;
[ call JVM_GetVersionInfo, etc. ]
JVM_VERSION_MAJOR(info.jvm_version) // => 25
JVM_VERSION_MINOR(info.jvm_version) // => 1
JVM_VERSION_MICRO(info.jvm_version) // => 6
JVM_VERSION_BUILD(info.jvm_version) // => 3

So, minor, micro would be wrong. Question is what's more likely in use
today in the wild? JVM_GetVersionInfo or sun.misc.Version.

> The current value for 7 is 402980866
> (0001 1000 0000 0101 0000 0000 0000 0010)
> which thus gets interpreted as above.
> 
> The new method encodes the same version numbers as 402720002
> (0001 1000 0000 0001 0000 0101 0000 0010)
> 
> which can be read back as major 24, minor 261, micro 0 and build 2 using
> the updated decoding.
> 
> If the old decoding is used, we get major 24, minor 1, micro 5 and build
> 2. As micro is usually set to zero, updated code should be able to use
> this to instead use the new interpretation, while, in the worst case,
> stale code will get a unique, if incorrect, version.

Yes.

So, Andrew Haley, what's the verdict on this one? There is a risk, yes.
Not fixing the bug seems riskier, though. Thoughts?

Thanks,
Severin

[1] https://bugs.openjdk.java.net/browse/JDK-8244548?focusedCommentId=14338519&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14338519