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

[harold seigel]

Hi,

Please review this new version of the fix for bug 6924920:

http://cr.openjdk.java.net/~hseigel/bug_6924920_3/ 
<http://cr.openjdk.java.net/%7Ehseigel/bug_6924920_3/>

This fix contains the following changes:

    1. Uses of a template to verify that the incoming buffer to function
    get_header_version() is the correct size.

    2. Calls a better hash function.

    3. Corrects a null termination problem.

Thank you,
Harold

On 11/16/2012 9:01 AM, harold seigel wrote:
> 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
>>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20121121/33cd31fe/attachment-0001.html