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

[Thomas Stüfe]

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