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,

That's a good point.  I'll add a check that the incoming argument is the 
expected length.

Thanks, Harold

On 11/15/2012 10:46 PM, David Holmes wrote:
> Hi Harold,
>
> The only nit I have with factoring the logic into a separate function 
> is that the function "knows" that only _jvm_ident is going to be 
> passed, as it assumes what the size is.
>
> Otherwise it seems functionally correct.
>
> David
>
> On 16/11/2012 12:20 AM, 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
>>>>>>