[8u] RFR(S): 8244548: JDK 8u: sun.misc.Version.jdkUpdateVersion() returns wrong result
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
On Thu, May 7, 2020 at 8:18 AM Severin Gehwolf <sgehwolf@redhat.com> wrote:
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 ;-)
Everyone expected 256 to be sufficient for update release numbering, before the gaps in the numbering scheme were introduced! I don't know how we ended up with 2 mod 10.
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. -- Andrew Haley (he/him) Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
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
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. 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. 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. As 7u releases are oddly numbered and increment in tens, we won't hit a case where the second byte (the old micro version) is zero. With 8u, this will happen where the update version is a multiple of 256 (e.g. 8u512, 8u1792, ...) Regarding the contract of this API, it is generally used only for interactions between the JVM and the class library. jvm.h is not included in the JDK image. Attempts to follow the API when CACAO & JamVM were still trying to work with OpenJDK suggest it has been unstable even across update releases [0] [1]. [0] https://icedtea.classpath.org/hg/icedtea7/rev/810d698ffdfb [1] https://icedtea.classpath.org/hg/icedtea7/rev/139f2a8f085d -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
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&p...
On 5/15/20 9:45 AM, Severin Gehwolf wrote:
So, Andrew Haley, what's the verdict on this one? There is a risk, yes. Not fixing the bug seems riskier, though. Thoughts?
Thanks for the detailed explanation. I think we have to keep sun.misc.Version working correctly but sacrifice binary consistency in JVM_GetVersionInfo. As long as stuff such as AOT compilation and class data sharing keep working I think we're OK. It's not great, but it's an almost-inevitable consequence of using a one-byte field for the minor version and bumping releases by 10. -- Andrew Haley (he/him) Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> https://keybase.io/andrewhaley EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
On Fri, 2020-05-15 at 14:29 +0100, Andrew Haley wrote:
On 5/15/20 9:45 AM, Severin Gehwolf wrote:
So, Andrew Haley, what's the verdict on this one? There is a risk, yes. Not fixing the bug seems riskier, though. Thoughts?
Thanks for the detailed explanation.
I think we have to keep sun.misc.Version working correctly but sacrifice binary consistency in JVM_GetVersionInfo. As long as stuff such as AOT compilation and class data sharing keep working I think we're OK. It's not great, but it's an almost-inevitable consequence of using a one-byte field for the minor version and bumping releases by 10.
Agreed. Thanks for the review. I've requested 8u approval for this bug. Cheers, Severin
On 15/05/2020 14:29, Andrew Haley wrote:
On 5/15/20 9:45 AM, Severin Gehwolf wrote:
So, Andrew Haley, what's the verdict on this one? There is a risk, yes. Not fixing the bug seems riskier, though. Thoughts?
Thanks for the detailed explanation.
I think we have to keep sun.misc.Version working correctly but sacrifice binary consistency in JVM_GetVersionInfo. As long as stuff such as AOT compilation and class data sharing keep working I think we're OK. It's not great, but it's an almost-inevitable consequence of using a one-byte field for the minor version and bumping releases by 10.
I see little to no risk in changing this. Anyone using 7u261 or later, or 8u262 or later, is going to get an overflowed minor JVM version and an overflowed upgrade JDK version even if we do nothing at all. What we can do is attempt to fix things in a way that doesn't change the size of the structure, just the interpretation of its contents, as proposed here. Calling code that adapts to the modified version will then be able to get the correct values. Code that doesn't will be just as broken as if we'd made no change. Arguably, even that case will be a little better, as with a non-zero micro value, the incorrect version will at least be distinct from older versions. This is for 7u & 8u, which don't have AOT compilation, so that shouldn't be an issue. I'll be approving this for 8u. Thanks, -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
participants (4)
-
Andrew Haley
-
Andrew Hughes
-
Martin Buchholz
-
Severin Gehwolf