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

Tue Nov 27 06:42:21 PST 2012

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