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

Thomas Stüfe thomas.stuefe at gmail.com
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 jioprint and jioprintf completely (along with calljioprint in ostream.cpp). Those 2 functions were just 2 hooks used in ostream.cpp and got routed through jiovfprintf anyway, except for the raw write case when no vfprinthook 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 jiovfprintf 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 calljioprint 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 "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