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

Tue Nov 27 07:00:05 PST 2012

Thanks Harold - this looks good.
Coleen

On 11/27/2012 09:42 AM, harold seigel wrote:
> Hi,
>
> Please review this new version of the fix for bug 6924920.  This 
> version replaces size_t with int and adds a comment.
>
> http://cr.openjdk.java.net/~hseigel/bug_6924920_4/ 
> <http://cr.openjdk.java.net/%7Ehseigel/bug_6924920_4/>
>
> Thanks, Harold
>
> On 11/26/2012 8:32 AM, harold seigel wrote:
>> Hi David,
>>
>> Thanks again for the review.  I used a template because I couldn't 
>> find a better way to check the size of the header_version buffer.
>>
>> Also, I'll look into changing the size_t to int.
>>
>> Thanks, Harold
>>
>> On 11/22/2012 10:03 PM, David Holmes wrote:
>>> On 22/11/2012 10:49 PM, David Holmes wrote:
>>>> On 22/11/2012 4:50 AM, harold seigel wrote:
>>>>> 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.
>>>>
>>>> Certainly more complex than I had expected - not that familiar with
>>>> template functions. But curiously for the second time today I see a
>>>> size_t variable being used in an array index expression:
>>>>
>>>> template <size_t N> static void get_header_version(char
>>>> (&header_version) [N]) {
>>>>
>>>> where it should be an int (according to ISO C standard).
>>>
>>> I humbly withdraw that comment. It is required to be an "integer 
>>> type" which includes both signed and unsigned and various sizes.
>>>
>>> I certainly prefer to use only int values. YMMV.
>>>
>>> David
>>> ------
>>>
>>>>> 2. Calls a better hash function.
>>>>>
>>>>> 3. Corrects a null termination problem.
>>>>
>>>> Seems okay.
>>>>
>>>> David
>>>> -----
>>>>
>>>>> 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
>>>>>>>>>>>>