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

[harold seigel]

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
>>>