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

David Holmes david.holmes at oracle.com
Wed Apr 18 22:10:49 UTC 2018


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


More information about the hotspot-dev mailing list