RFR: 8146793: logStream::write re-formats string

[Kim Barrett]

On Jan 19, 2016, at 9:06 PM, John Rose <john.r.rose at oracle.com> wrote:
> 
> On Jan 19, 2016, at 5:07 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> 
>> It doesn't look like the logging code has anything like that. I think
>> it would need to be an addition to Log<>::vwrite. [...] I'll file an RFE for this.
> 
> Thank you.  If you get inspired to roll this in now, I'd support that.

I decided not to take that on right now, instead filing
https://bugs.openjdk.java.net/browse/JDK-8147866

>>> Nice catches on the va_copy and buffer overrun.  How did you find
>>> the log_func(foo) instead of log_func("%s", foo)?  Please tell me it
>>> was a gcc warning; if not then we have some software rot going on.
>> 
>> Sorry, but I found it the hard way, via a crash (I think it was the
>> assert in Log<>::vwrite). logStream::_log_func is a function pointer.
>> Maybe we can attach an ATTRIBUTE_PRINTF to the data member and/or to
>> the logStream constructor argument? And maybe that would produce the
>> desired warnings? I think attaching that attribute would have
>> generated a warning for the non-literal format string being passed to
>> _log_func.
> 
> OK, yes; that's what I mean by software rot.  We went to a lot of
> trouble to annotate and fix the the format problems, and now they
> are building up again as people add new functions without the
> annotation.  Putting in the annotations is really part of making
> a firm fix to the "%s" problem, not just reacting to crashers as
> they show up.  Fix the root cause, not just the crash.

Agreed that we want to maintain our format attribute annotations. The
logging package appears to have that. It looks like this one case of a
pointer to a format function was simply overlooked.

I tried adding the attribute with the original code, and indeed it does result in a warning:

error: format not a string literal and no format arguments [-Werror=format-security]

Which is a useful warning here; correct diagnosis and handling of the
warning would have prevented the re-format bug.

So here’s a new set of webrevs:
full:  http://cr.openjdk.java.net/~kbarrett/8146793/webrev.01/
incr:  http://cr.openjdk.java.net/~kbarrett/8146793/webrev.01.inc/