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

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 "calljioprint" which aims to ensure that the string we hand down to jioprint is zero terminated. But ultimately, the problem was never jioprint, 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': 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. 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 jioprint and jioprintf from jvm.cpp and doing all in ostream.cpp. At the one place where we would call jioprint 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 jioprintf is employed, we can just call jiovfprintf(defaultStream::outputstream()). I don't see any other users of jioprint and jioprintf in the jdk repo.

Any objections to that (e.g. some Oracle closed coding that'd break)? Otherwise I'll prepare something... $ grep -r jioprint closed Showed nothing found. I've prepared a new webrev: http://cr.openjdk.java.net/~clanger/webrevs/8201649.1/ I remove jioprintf and jioprint 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 jiofprintf. Furhtermore, I have updated the jioprint 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