Request for review 6924920: Class Data Sharing limit on the java version string can create failures: JVM Ident field too long

[Peter Levart]

Hi Harold,

Isn't this line:

  106     header_version[JVM_IDENT_MAX] = 0;  // Null terminate.

...going to cause memory corruption? I think it should be:

  106     header_version[JVM_IDENT_MAX-1] = 0;  // Null terminate.


Regards, Peter

On 11/15/2012 03:20 PM, harold seigel wrote:
> Please review these updated changes to fix bug 6924920.  The proposed 
> changes implement what was discussed in my 10/11/2012 reply to Peter.
>
> The updated changes are available at 
> http://cr.openjdk.java.net/~hseigel/bug_6924920_2/ 
> <http://cr.openjdk.java.net/%7Ehseigel/bug_6924920_2/>
>
> This implementation was straightforward because it used an existing vm 
> hash function.
>
> Thanks, Harold
>
> On 11/13/2012 5:05 PM, harold seigel wrote:
>> HI David,
>>
>> Thanks for your comments.  However, we still think that appending a 
>> hash code is worth doing.  As you point out, this is not a fool proof 
>> scheme because of potential hash collisions, but it will provide a 
>> stronger sanity check than just comparing truncated strings.
>>
>> Also, since this hash will only occur when writing to or opening the 
>> shared area, the cost of doing the hash is not significant.
>>
>> I plan to send out new code to review this week.
>>
>> Thanks, Harold
>>
>> On 11/9/2012 6:12 PM, David Holmes wrote:
>>> On 10/11/2012 6:08 AM, harold seigel wrote:
>>>> Hi Peter,
>>>>
>>>> We plan to use something similar to what you suggested. If the version
>>>> string is < JVM_IDENT_MAX then it will not get truncated. Any version
>>>> string >= JVM_IDENT_MAX will get truncated and the last N bytes of the
>>>> string will be filled in with hex digits from a hash. Code that then
>>>> reads this version string can tell whether or not the string has been
>>>> hashed based on its length.
>>>
>>> This seems excessive. You could still get a hash collision and the 
>>> likelihood of that depends on the hashing algorithm and the likely 
>>> differences in such long version strings (which you don't know). 
>>> Plus you can always build two quite different VMs that have the same 
>>> version string!
>>>
>>> As I understand it this is meant to be a basic sanity check not a 
>>> foolproof identification scheme - if you want the latter then 
>>> generate a GUID on every build and use that.
>>>
>>> David
>>> -----
>>>
>>>> Thanks for the suggestion!
>>>> Harold
>>>>
>>>> On 11/9/2012 11:46 AM, Peter Levart wrote:
>>>>> On 11/09/2012 05:40 AM, David Holmes wrote:
>>>>>> On 9/11/2012 5:48 AM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>> Harold,
>>>>>>> I looked at this once and thought it was okay, but now I don't 
>>>>>>> think it
>>>>>>> is. I think you want fail_stop() but just not to print the 
>>>>>>> string that
>>>>>>> isn't initialized.
>>>>>>> fail_continue() is used when reading the archive. It allows the 
>>>>>>> vm to
>>>>>>> continue without using the shared archive. But when you're 
>>>>>>> dumping it,
>>>>>>> if the string is truncated, it won't create a usable shared archive
>>>>>>> anyway. The fail_continue() closes the CDS archive file.
>>>>>>>
>>>>>>> If you want to have a truncated string work, you have issue the
>>>>>>> fail_continue() message as a warning only. Then change the code for
>>>>>>> Xshare:on that validates that the versions match to truncate the
>>>>>>> current_version() too.
>>>>>>>
>>>>>>> I suggest leaving it as fail_stop() but fixing the message to 
>>>>>>> not print
>>>>>>> uninitialized data.
>>>>>>
>>>>>> I agree that fail_continue is the wrong choice. But it seems to me
>>>>>> that when the ident is truncated all that is needed is a warning
>>>>>> printed. AFAICS the archive will still be usable with a truncated
>>>>>> ident and it will match when read back in during validate().
>>>>>
>>>>> Hi,
>>>>>
>>>>> But wouldn't also match when read back with a version string that
>>>>> differs from the writer's version string only in chars past 255? Are
>>>>> those chars significant?
>>>>>
>>>>> What about truncating the version string to 255-32 = 223 bytes and
>>>>> appending (only if truncated) a 32 hex digits of a MD5 of the whole
>>>>> version string? No warnings.
>>>>>
>>>>> Regards, Peter
>>>>>
>>>>>>
>>>>>> David
>>>>>> ------
>>>>>>
>>>>>>>
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 11/8/2012 1:23 PM, harold seigel wrote:
>>>>>>>> Please review the following change.
>>>>>>>>
>>>>>>>> Summary: The problem was fixed by truncating the JVM ident to
>>>>>>>> JVM_IDENT_MAX and calling method fail_continue() instead of
>>>>>>>> fail_stop().
>>>>>>>>
>>>>>>>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_6924920/
>>>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_6924920/>
>>>>>>>>
>>>>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=6924920
>>>>>>>>
>>>>>>>> The changes were tested with JPRT, JCK, and by using a version 
>>>>>>>> string
>>>>>>>> that exceeded 256 characters.
>>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>