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

[John Rose]

Yes, that's it. Thanks!

– John

> On Jan 20, 2016, at 1:17 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> 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/
>