RFR(S): 8201649: Remove dubious call_jio_print in ostream.cpp

Thu Apr 19 09:12:22 UTC 2018

Hi Christoph,

On Thu, Apr 19, 2018 at 10:00 AM, Langer, Christoph
<christoph.langer at sap.com> wrote:
> Hi David, Thomas,
>
> @David: No, I don't think it needs to be dialed back at this point, but I agree, the bug should be updated somewhat to the current scope by mentioning that the goal is to get rid of jio_print and jio_printf completely (along with call_jio_print in ostream.cpp). Those 2 functions were just 2 hooks used in ostream.cpp and got routed through jio_vfprintf anyway, except for the raw write case when no vfprint_hook was installed - and we'll preserve that.
>

Hm. Instead of broading the scope of the issue, I would actually
prefer the bug description to be clarified and limited to the original
problem we tried to solve. Because that was a real valid problem.

If only to get David's full review and not to confuse him further :)

Just to recuperate, the original problem was:

We have situations where we would like to feed non-zero-terminated
strings with a given string length to jio_print(). One of these
occasions is when the attach framework tries to write multiple lines
via defaultStream::print_raw().

But jio_print() takes a zero terminated string. Non-zero terminated
strings are therefore zero terminated by creating a copy of the string
and adding a zero - see call_jio_helper().

This copying happens with a fixed buffer and sometimes causes
truncation. These issues are hard to find, since they seem to be
random - if the input string just happens to be followed by a zero,
the copy step is avoided and no truncation happens.

Christophs fix aims to get rid of the copying step to avoid
truncation. All the rest is fluff and cleanup.

> As for your concern, Thomas: I don't think this should hold us back here. The jio_vfprintf function is used ubiquitously throughout the jdk and the same concern would apply. Though obviously the vfprintf hook functionality is not documented very well, I guess in fact we rely on Posix compliant vfprintf implementations already for quite some time. Plus we are not talking about a downport into a delivered JDK here but it's about the upcoming JDK11.
>

Okay...

Best regards, Thomas

> Best regards
> Christoph
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Donnerstag, 19. April 2018 00:11
>> To: Thomas Stüfe <thomas.stuefe at gmail.com>; Langer, Christoph
>> <christoph.langer at sap.com>
>> Cc: daniel.daugherty at oracle.com; hotspot-dev at openjdk.java.net; hotspot-
>> runtime-dev at openjdk.java.net
>> Subject: Re: RFR(S): 8201649: Remove dubious call_jio_print in ostream.cpp
>>
>> I'm losing track of the original goal here and the scope of this seems
>> to be blowing out into unknown territory. We don't know why we need the
>> raw write; and we don't know what we can assume about the vprintf-hook.
>>
>> I suggest to dial this back to whatever real issue was initially being
>> addressed.
>>
>> Cheers,
>> David
>>
>> On 19/04/2018 1:55 AM, Thomas Stüfe wrote:
>> > Hi Chrisoph,
>> >
>> > thanks for the new webrev, this looks all very reasonable.
>> >
>> > Unfortunately a small issue occurred to me while thinking this over...
>> > Sigh, this turns out to be more complicated than I thought.
>> >
>> > The original intent of this change was to get rid of that extra copy
>> > step we do in "call_jio_print" which aims to ensure that the string we
>> > hand down to jio_print is zero terminated.
>> >
>> > But ultimately, the problem was never jio_print, but the vfprintf
>> > hook: The vfprintf hook has the form "foo(FILE*, char* fmt, ....) and
>> > this is a contract with an embedding program - it has to provide a
>> > "fprinf-like function" but ultimately can do whatever it wants with
>> > the arguments we give it.
>> >
>> > (IMHO this whole vfprintf hook thing was very badly designed. We had a
>> > number of issues with this already. For a discussion about some
>> > details, see e.g.
>> > http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-
>> May/026794.html
>> > .)
>> >
>> > The problem I see is that now we may call the vfprintf hook with
>> > something like ("%.*s", precision, s) with the explicit intention of
>> > passing in not-zero-terminated strings for s and a precision which
>> > prevents us reading beyond the end of s. This was the whole point - we
>> > avoid the copy step.
>> >
>> > As we discussed offlist, this would be a perfectly valid fix were we
>> > to feed those arguments to a standard fprintf() function which is
>> > POSIX compatible, because POSIX states for the format specifier 's':
>> >
>> > <quote>
>> > The argument shall be a pointer to an array of char. Bytes from the
>> > array shall be written up to (but not including) any terminating null
>> > byte. If the precision is specified, no more than that many bytes
>> > shall be written. If the precision is not specified or is greater than
>> > the size of the array, the application shall ensure that the array
>> > contains a null byte.
>> > </quote>
>> >
>> > This explicitly allows us to pass a pointer to a non-zero-terminated
>> > string as long as the precision is smaller than its length.
>> >
>> > However, the argument vfprintf hook can be anything. It is a
>> > user-provided function. Usually they will probably just call vxxprintf
>> > functions from the libc, but for all we know they may roll their own
>> > printf routine. So, we may uncover bugs in their implementation.
>> >
>> > Seeing that the vfprintf hook is very badly documented, we move in
>> > gray area here. We may break user code which has a bad, non-Posix
>> > implementation of the vfprintf hook.
>> >
>> > I would like to know if others think this concern is valid.
>> >
>> > Otherwise, the patch looks good.
>> >
>> >
>> > ..Thomas
>> >
>> >
>> >
>> > On Wed, Apr 18, 2018 at 5:01 PM, Langer, Christoph
>> > <christoph.langer at sap.com> wrote:
>> >> Hi,
>> >>
>> >>>> I think it could be cleaned up nicely by removing jio_print and jio_printf
>> >>> from jvm.cpp and doing all in ostream.cpp. At the one place where we
>> would
>> >>> call jio_print after my patch, we can do the vfprintf hook check and then
>> >>> either write to the hook or do the raw write. And the other 2 places
>> where
>> >>> jio_printf is employed, we can just call
>> >>> jio_vfprintf(defaultStream::output_stream()). I don't see any other
>> users of
>> >>> jio_print and jio_printf in the jdk repo.
>> >>>>
>> >>>> Any objections to that (e.g. some Oracle closed coding that'd break)?
>> >>> Otherwise I'll prepare something...
>> >>>
>> >>> $ grep -r jio_print closed
>> >>>
>> >>> Showed nothing found.
>> >>
>> >> I've prepared a new webrev:
>> http://cr.openjdk.java.net/~clanger/webrevs/8201649.1/
>> >>
>> >> I remove jio_printf and jio_print completely.
>> >>
>> >> If we agree that in defaultStream::write(), the raw write is not needed
>> and we can use fprintf, we could further simplify the code to just  call
>> jio_fprintf.
>> >>
>> >> Furhtermore, I have updated the jio_*print* function declarations in
>> jvm.cpp to match those of jvm.h. Is that desired (I thought yes, in times
>> where we try to get rid of the mapfiles anyway). I wonder if those symbols
>> can be taken out of make/hotspot/symbols/symbols-shared then...
>> >>
>> >> Best regards
>> >> Christoph