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

[harold seigel]

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