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

[harold seigel]

Hi Peter,

I agree, and will change it.

Thanks!
Harold

On 11/19/2012 2:58 AM, Peter Levart wrote:
> 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
>>>>>>
>