RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

Schmelter, Ralf
Please review this fix. It applies the same correction to jstack and jinfo which was already applied to jcmd. To avoid code duplication, I moved the actual code to a utility class.

webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8227868/webrev.0/
bugreport: https://bugs.openjdk.java.net/browse/JDK-8227868

Best regards,
Ralf
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

Severin Gehwolf
On Thu, 2019-07-18 at 10:07 +0000, Schmelter, Ralf wrote:
> Please review this fix. It applies the same correction to jstack and
> jinfo which was already applied to jcmd. To avoid code duplication, I
> moved the actual code to a utility class.
>
> webrev:
> http://cr.openjdk.java.net/~rschmelter/webrevs/8227868/webrev.0/
> bugreport: https://bugs.openjdk.java.net/browse/JDK-8227868

+    /**
+     * Reads characters in UTF-8 format from the input stream and prints them
+     * with the given print stream. Closes the input stream before it returns.
+     *
+     * @return The number of printed characters.
+     */
+    public static long drainUTF8(InputStream is, PrintStream ps) throws IOException {
+        long result = 0;
+
+        try (BufferedInputStream bis = new BufferedInputStream(is);
+             InputStreamReader isr = new InputStreamReader(bis, "UTF-8")) {
+            char c[] = new char[8192];
+            int n;
+
+            do {
+                n = isr.read(c);
+
+                if (n > 0) {
+                    result += n;
+                    System.out.print(n == c.length ? c : Arrays.copyOf(c, n));
+                }

If I read this right, then the method doesn't do what it claims it
does. It's printing to System.out unconditionally instead of to 'ps'.
What am I missing?

Would it be possible to add regression tests for this? Along the lines
of test/jdk/sun/tools/jstack/BasicJStackTest.java

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

RE: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

Schmelter, Ralf
Hi Severin,

thanks for the review.

> If I read this right, then the method doesn't do what it claims it
> does. It's printing to System.out unconditionally instead of to 'ps'.
> What am I missing?

Nothing. The code should of course use ps and not System.out. I've updated the webrev:
http://cr.openjdk.java.net/~rschmelter/webrevs/8227868/webrev.1/

I'm not sure why additional tests would be needed, given that the existing ones already test the code.

Best regards,
Ralf
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

Severin Gehwolf
Hi Ralf,

On Thu, 2019-07-18 at 12:24 +0000, Schmelter, Ralf wrote:

> Hi Severin,
>
> thanks for the review.
>
> > If I read this right, then the method doesn't do what it claims it
> > does. It's printing to System.out unconditionally instead of to 'ps'.
> > What am I missing?
>
> Nothing. The code should of course use ps and not System.out. I've updated the webrev:
> http://cr.openjdk.java.net/~rschmelter/webrevs/8227868/webrev.1/

Thanks.

> I'm not sure why additional tests would be needed, given that the existing ones already test the code.

I meant a test which reproduces the issue. It appears existing tests
don't expose the issue (as they are likely not using those characters).
Neither JDK-8222491 nor this fix includes a regression test. How would
we ensure this doesn't break in future? Is this a case of being hard to
reproduce?

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

David Holmes
In reply to this post by Schmelter, Ralf
Hi Ralf,

On 18/07/2019 10:24 pm, Schmelter, Ralf wrote:

> Hi Severin,
>
> thanks for the review.
>
>> If I read this right, then the method doesn't do what it claims it
>> does. It's printing to System.out unconditionally instead of to 'ps'.
>> What am I missing?
>
> Nothing. The code should of course use ps and not System.out. I've updated the webrev:
> http://cr.openjdk.java.net/~rschmelter/webrevs/8227868/webrev.1/

General approach seems okay. Using Arrays.copyOf is better than creating
Strings just for the purposes of printing. Not sure why you chose a 8K
buffer when existing codes uses 256 chars?

> I'm not sure why additional tests would be needed, given that the existing ones already test the code.

There would ideally have been a regression test written for JDK-8222491
to demonstrate the conversion problem. Such a test would then be
modified to extend to this bug. If it is not practical to try and
produce a regression test to the demonstrate the problem then the bug
reports should have the noreg-hard label added to them. (I doubt we have
any current tests using non-ascii UTF8 :( ).

Thanks,
David
-----

> Best regards,
> Ralf
>
Reply | Threaded
Open this post in threaded view
|

RE: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

Schmelter, Ralf
Hi David,

thanks for the review.

> Not sure why you chose a 8K
> buffer when existing codes uses 256 chars?

I took it from the Reader.transferTo() method. But I've changed it to back to 256.

> There would ideally have been a regression test written for JDK-8222491
> to demonstrate the conversion problem.

I've added a test which shows the problem for jcmd. And I've modified the basic jstack test to test for the problem too. For jinfo testing would be much harder, since it either prints already sanitized output (system properties) or values which depend on the platform encoding (flags and command line).

Here is the update webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8227868/webrev.3/

Best regards,
Ralf
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

Chris Plummer
Hi Ralf,

The indent is off for the following in BasicJStackTest.java:

   39         private static String markerName = "markerName" +
"\u00e4\u0bb5".repeat(10_000);

Also you need to update the copyright date for BasicJStackTest.java,
JInfo.java ,and JStack.java.

Otherwise looks good. No need for another weberv.

thanks,

Chris

On 7/19/19 6:33 AM, Schmelter, Ralf wrote:

> Hi David,
>
> thanks for the review.
>
>> Not sure why you chose a 8K
>> buffer when existing codes uses 256 chars?
> I took it from the Reader.transferTo() method. But I've changed it to back to 256.
>
>> There would ideally have been a regression test written for JDK-8222491
>> to demonstrate the conversion problem.
> I've added a test which shows the problem for jcmd. And I've modified the basic jstack test to test for the problem too. For jinfo testing would be much harder, since it either prints already sanitized output (system properties) or values which depend on the platform encoding (flags and command line).
>
> Here is the update webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8227868/webrev.3/
>
> Best regards,
> Ralf



Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

David Holmes
In reply to this post by Schmelter, Ralf
Hi Ralf,

On 19/07/2019 11:33 pm, Schmelter, Ralf wrote:
> Hi David,
>
> thanks for the review.
>
>> Not sure why you chose a 8K
>> buffer when existing codes uses 256 chars?
>
> I took it from the Reader.transferTo() method. But I've changed it to back to 256.

Okay.

>> There would ideally have been a regression test written for JDK-8222491
>> to demonstrate the conversion problem.
>
> I've added a test which shows the problem for jcmd. And I've modified the basic jstack test to test for the problem too. For jinfo testing would be much harder, since it either prints already sanitized output (system properties) or values which depend on the platform encoding (flags and command line).

The new test for Jcmd passes for me (linux x64 fastdebug) even without
the fix. But the modified jstack test fails.

The new test has the wrong id on the @bug line.

Thanks,
David

> Here is the update webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8227868/webrev.3/
>
> Best regards,
> Ralf
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

Severin Gehwolf
Hi David,

On Mon, 2019-07-22 at 14:13 +1000, David Holmes wrote:
> Hi Ralf,
>
> On 19/07/2019 11:33 pm, Schmelter, Ralf wrote:
> > I've added a test which shows the problem for jcmd. And I've modified the basic jstack test to test for the problem too. For jinfo testing would be much harder, since it either prints already sanitized output (system properties) or values which depend on the platform encoding (flags and command line).
>
> The new test for Jcmd passes for me (linux x64 fastdebug) even without
> the fix. But the modified jstack test fails.

That's probably because you'd have to backout JDK-8222491 first for it
to fail. JDK-8222491 got pushed without a reproducer. Note that the new
test references the *right* bug for that reason.

Thanks,
Severin

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8227868: jinfo and jstack can fail converting UTF8 output to strings

David Holmes
On 22/07/2019 6:24 pm, Severin Gehwolf wrote:

> Hi David,
>
> On Mon, 2019-07-22 at 14:13 +1000, David Holmes wrote:
>> Hi Ralf,
>>
>> On 19/07/2019 11:33 pm, Schmelter, Ralf wrote:
>>> I've added a test which shows the problem for jcmd. And I've modified the basic jstack test to test for the problem too. For jinfo testing would be much harder, since it either prints already sanitized output (system properties) or values which depend on the platform encoding (flags and command line).
>>
>> The new test for Jcmd passes for me (linux x64 fastdebug) even without
>> the fix. But the modified jstack test fails.
>
> That's probably because you'd have to backout JDK-8222491 first for it
> to fail. JDK-8222491 got pushed without a reproducer. Note that the new
> test references the *right* bug for that reason.

Ah! Sorry. Thanks for explaining.

David

> Thanks,
> Severin
>