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

[David Holmes]

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

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