PING: RFR: 8204531: Remove unused chars following '\0'

[Thomas Stüfe]

Disclaimer: I am Reviewer but not versed in this corner of the jdk,
nor have knowledge of the history, so take what I suggest with a grain
of salt.

That said, I'd probably just add a variant to
CStringUtilities::getString which takes encoding as an argument, and
then pass "US-ASCII" to remain compatible with the current version of
jsnap.

..Thomas

On Wed, Jun 13, 2018 at 2:37 PM, Yasumasa Suenaga <yasuenag at gmail.com> wrote:
> Hi Thomas,
>
> Thank you for your comment.
>
> CStringUtilities.java says the String which is constructed from byte array
> should use Charset.defaultCharset(). [1]
> I'm not convinced how should we fix - US-ASCII or default charset.
>
> IMHO we should use default charset. However I concern I need to get CSR
> approvement.
> (In addition, I want to refactor CStringUtilities. I guess we can make more
> simple code :-)
>
>
> What do you think about it?
>
> Yasumasa
>
>
> [1]
> http://hg.openjdk.java.net/jdk/jdk/file/7bf4f1b5e438/src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/CStringUtilities.java#l73
>
>
>
> On 2018/06/13 21:18, Thomas Stüfe wrote:
>>
>> Hi Yasumasa,
>>
>> your patch will change behavior of jsnap if file.encoding is set to
>> something different than US-ASCII.
>>
>> Old version uses, hard wired, US-ASCII.
>>
>> You now use jvm/hotspot/utilities/CStringUtilities.java, which uses
>>
>> private static String encoding = System.getProperty("file.encoding",
>> "US-ASCII");
>>
>> and thus may use a different encoding.
>>
>> Best Regards, Thomas
>>
>>
>>
>> On Wed, Jun 13, 2018 at 2:09 PM, Yasumasa Suenaga <yasuenag at gmail.com>
>> wrote:
>>>
>>> PING: Could you review it?
>>> I want to merge this change before JDK 11 RDP 1.
>>>
>>>>>>      webrev:
>>>>>>        http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.00/
>>>>>>
>>>>>>      JBS:
>>>>>>        https://bugs.openjdk.java.net/browse/JDK-8204531
>>>
>>>
>>>
>>>
>>> Yasumasa
>>>
>>>
>>> On 2018/06/08 15:38, Yasumasa Suenaga wrote:
>>>>
>>>>
>>>> Hi David,
>>>>
>>>> 2018-06-08 10:47 GMT+09:00 David Holmes <david.holmes at oracle.com>:
>>>>>
>>>>>
>>>>> Hi Yasumasa,
>>>>>
>>>>> On 7/06/2018 10:43 PM, Yasumasa Suenaga wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this change:
>>>>>>
>>>>>>      webrev:
>>>>>>        http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.00/
>>>>>>
>>>>>>      JBS:
>>>>>>        https://bugs.openjdk.java.net/browse/JDK-8204531
>>>>>>
>>>>>>
>>>>>> We can use `jhsdb jsnap` to check all PerfData.
>>>>>> String values in PerfData are defined as jbyte array, but JSnap cannot
>>>>>> handle it well as following:
>>>>>>
>>>>>> ```
>>>>>> $ jhsdb jsnap --pid 28542 --all | less
>>>>>>
>>>>>> sun.gc.cause=No
>>>>>>
>>>>>>
>>>>>> GC^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@^@
>>>>>> ```
>>>>>>
>>>>>> You can see this value via `less` and `vim` on Linux. `^@` shows it is
>>>>>> non-ascii character.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> So your synopsis says "remove unused chars following \0". Is that
>>>>> really
>>>>> what is happening? I would have expected "new String(bytes, encoding)"
>>>>> to
>>>>> stop processing bytes when it encounters a NUL!
>>>>
>>>>
>>>>
>>>> String c'tor will continue to parse byte array even if it finds '\0'.
>>>> You can see it on JShell as below:
>>>>
>>>> -------------
>>>> jshell> var bytes = new byte[]{(byte)'a', (byte)'b', (byte)'c',
>>>> (byte)'\0', (byte)'d', (byte)'e', (byte)'f'}
>>>> bytes ==> byte[7] { 97, 98, 99, 0, 100, 101, 102 }
>>>>
>>>> jshell> new String(bytes, "US-ASCII")
>>>> $2 ==> "abc\000def"
>>>> -------------
>>>>
>>>>
>>>>>> PerfDataEntry has null-terminated C string. So we should restore as it
>>>>>> in
>>>>>> Java layer.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> If the issue is really "junk" beyond the \0 nul-terminator, how did we
>>>>> get
>>>>> such values? Shouldn't the byte array already be terminated at the NUL?
>>>>> Or
>>>>> is this just a raw representation of an array in the VM using the exact
>>>>> length of the array regardless of content?
>>>>
>>>>
>>>>
>>>> I think JSnap shows the array regardless of content.
>>>>
>>>>
>>>>> I see CStringUtilities.getString() both stops when it encounters a 0
>>>>> value,
>>>>> and uses the default file encoding (else ASCII).
>>>>>
>>>>> The fix seems perfectly reasonable, I'm just unclear where exactly this
>>>>> "bad" data should have been stopped.
>>>>
>>>>
>>>>
>>>> Thanks!
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>
>>>
>