Fix memory leak in printf_positional

  * stdio-common/vfprintf.c (printf_positional):
Free temporary data allocated via malloc.
---
 ChangeLog               | 4 ++++
 stdio-common/vfprintf.c | 2 ++
 2 files changed, 6 insertions(+)
  

For memory leak bugs, if possible we add testcases to the testsuite that 
use mtrace.  That means (in addition to listing the test in tests as 
usual) adding $(objpfx)<test>-mem.out to tests-special, setting <test>-ENV 
to set MALLOC_TRACE, and adding the makefile rule that calls mtrace; see 
various existing examples.  (Of course the test should be something 
verified to fail the mtrace tests before the leak fix is applied; 
presumably it isn't hard to construct such a case that uses the malloc 
path here, and there might even be an existing test that could just have 
the pieces to use mtrace added to it.)
  

On 08/26/2015 05:34 AM, Joseph Myers wrote:
> For memory leak bugs, if possible we add testcases to the testsuite that 
> use mtrace.  That means (in addition to listing the test in tests as 
> usual) adding $(objpfx)<test>-mem.out to tests-special, setting <test>-ENV 
> to set MALLOC_TRACE, and adding the makefile rule that calls mtrace; see 
> various existing examples.  (Of course the test should be something 
> verified to fail the mtrace tests before the leak fix is applied; 
> presumably it isn't hard to construct such a case that uses the malloc 
> path here, and there might even be an existing test that could just have 
> the pieces to use mtrace added to it.)
 
To repeat what I said in the bugzilla, which I should have just said here.

I agree, but I don't think anyone should spend more than an hour trying
to find such a test case. The static analysis tools can show you a failure,
but actually triggering that failure involves getting coverage over that
line of code, which isn't always trivial. We won't know until we try,
but I'd like anyone looking at the coverity results to be practical
about how possible it is to write a test case to cover the detected
failure.

Cheers,
Carlos.
  

On Thu, 27 Aug 2015, Carlos O'Donell wrote:

> I agree, but I don't think anyone should spend more than an hour trying
> to find such a test case. The static analysis tools can show you a failure,

I really don't think this case should take that long to find how to 
trigger the leak.

There will of course be cases where the reason for no test case is 
something like "this bug only appears if you allocate memory occupying at 
least 3/4 of the address space, which is impossible on most current 64-bit 
systems and unreasonable for the testsuite to do on 32-bit systems even if 
possible" - or cases where it's unclear whether any combination of 
circumstances can cause the code in question to be reached in practice.  
But the starting point for most user-visible bug fixes should be that a 
test is added if it's straightforward to figure out how to test for the 
bug reliably within the context of the existing test infrastructure.
  

On 08/28/2015 09:55 AM, Joseph Myers wrote:
> On Thu, 27 Aug 2015, Carlos O'Donell wrote:
> 
>> I agree, but I don't think anyone should spend more than an hour trying
>> to find such a test case. The static analysis tools can show you a failure,
> 
> I really don't think this case should take that long to find how to 
> trigger the leak.
> 
> There will of course be cases where the reason for no test case is 
> something like "this bug only appears if you allocate memory occupying at 
> least 3/4 of the address space, which is impossible on most current 64-bit 
> systems and unreasonable for the testsuite to do on 32-bit systems even if 
> possible" - or cases where it's unclear whether any combination of 
> circumstances can cause the code in question to be reached in practice.  
> But the starting point for most user-visible bug fixes should be that a 
> test is added if it's straightforward to figure out how to test for the 
> bug reliably within the context of the existing test infrastructure.

Agreed.

c.
  

On Fri, Aug 28, 2015 at 6:59 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/28/2015 09:55 AM, Joseph Myers wrote:
>> On Thu, 27 Aug 2015, Carlos O'Donell wrote:
>>
>>> I agree, but I don't think anyone should spend more than an hour trying
>>> to find such a test case. The static analysis tools can show you a failure,
>>
>> I really don't think this case should take that long to find how to
>> trigger the leak.

It doesn't take very long: all I needed is a printf invocation with >=
65536 / 3 / sizeof(void*) arguments.

Writing such invocation by hand is of course toublesome, plumbing
Makefile to generate it for me, and figuring out why it doesn't work
is what takes time :-(

In addition, there is a GCC regression: compiling a printf call with
2800 arguments takes 4.8.4-2ubuntu1~14.04 0.06s without optimization,
0.86s with -O2. Same numbers for current GCC trunk (@r227321): 0.06s
and  4m46s. This is on a very recent and fast PC. I expect there could
be PCs in current use where the time will be 3x longer.

Are we willing to tolerate such long compile times for a test case?
If so, I'll send a combined patch.

Thanks,
  

On Sat, Aug 29, 2015 at 11:38 AM, Paul Pluzhnikov
<ppluzhnikov@google.com> wrote:

> In addition, there is a GCC regression:

I meant to say "performance regression". I filed
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67396 with details for
that.
  

On Sat, Aug 29, 2015 at 11:38:13AM -0700, Paul Pluzhnikov wrote:
> On Fri, Aug 28, 2015 at 6:59 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> > On 08/28/2015 09:55 AM, Joseph Myers wrote:
> >> On Thu, 27 Aug 2015, Carlos O'Donell wrote:
> >>
> >>> I agree, but I don't think anyone should spend more than an hour trying
> >>> to find such a test case. The static analysis tools can show you a failure,
> >>
> >> I really don't think this case should take that long to find how to
> >> trigger the leak.
> 
> It doesn't take very long: all I needed is a printf invocation with >=
> 65536 / 3 / sizeof(void*) arguments.
> 
> Writing such invocation by hand is of course toublesome, plumbing
> Makefile to generate it for me, and figuring out why it doesn't work
> is what takes time :-(
> 
> In addition, there is a GCC regression: compiling a printf call with
> 2800 arguments takes 4.8.4-2ubuntu1~14.04 0.06s without optimization,
> 0.86s with -O2. Same numbers for current GCC trunk (@r227321): 0.06s
> and  4m46s. This is on a very recent and fast PC. I expect there could
> be PCs in current use where the time will be 3x longer.
> 
> Are we willing to tolerate such long compile times for a test case?
> If so, I'll send a combined patch.
>
There is also problem what this would catch, as in future a problem
could be with 65536 arguments or other magical constant this isn't that
much useful.
  

On Sat, Aug 29, 2015 at 9:29 PM, Ondřej Bílka <neleai@seznam.cz> wrote:

> There is also problem what this would catch, as in future a problem
> could be with 65536 arguments or other magical constant this isn't that
> much useful.

Sorry, you lost me.

The test case catches the existing bug that the patch at the start of
this thread fixes.

I am sure there other bugs that this test doesn't catch, and I am sure
there could be new bugs in the future that require more arguments to
catch. I don't think these are good reasons not to have a test.
  

On Sat, 29 Aug 2015, Paul Pluzhnikov wrote:

> It doesn't take very long: all I needed is a printf invocation with >=
> 65536 / 3 / sizeof(void*) arguments.
> 
> Writing such invocation by hand is of course toublesome, plumbing
> Makefile to generate it for me, and figuring out why it doesn't work
> is what takes time :-(

Normally such invocations would be generated with macros, e.g.

#define A a, a, a, a, a, a, a, a, a, a
#define B A, A, A, A, A, A, A, A, A, A

etc., unless the sort of expansion you require is unsuited to that for 
some reason.

> In addition, there is a GCC regression: compiling a printf call with
> 2800 arguments takes 4.8.4-2ubuntu1~14.04 0.06s without optimization,
> 0.86s with -O2. Same numbers for current GCC trunk (@r227321): 0.06s
> and  4m46s. This is on a very recent and fast PC. I expect there could
> be PCs in current use where the time will be 3x longer.

To me this suggests building the test with -O0 (or -fno-<something>, if 
the responsible optimization has been identified and can be disabled that 
way), with a comment in the Makefile pointing to the GCC bug and naming a 
GCC version as of which it appears (and a version as of which it is fixed, 
if applicable).
  

On Sat, Aug 29, 2015 at 09:35:43PM -0700, Paul Pluzhnikov wrote:
> On Sat, Aug 29, 2015 at 9:29 PM, Ondřej Bílka <neleai@seznam.cz> wrote:
> 
> > There is also problem what this would catch, as in future a problem
> > could be with 65536 arguments or other magical constant this isn't that
> > much useful.
> 
> Sorry, you lost me.
> 
> The test case catches the existing bug that the patch at the start of
> this thread fixes.
> 
> I am sure there other bugs that this test doesn't catch, and I am sure
> there could be new bugs in the future that require more arguments to
> catch. I don't think these are good reasons not to have a test.
> 
Just that it doesn't make sense to add specific test for bugs that have very
narrow use case and are fixed as it won't catch similar ones with sligth
variation of parameters. Instead I would try to make generic tests that
would catch wider class of bugs.