Message ID | 1440571295-20230-1-git-send-email-eggert@cs.ucla.edu |
---|---|
State | Superseded |
Headers |
Received: (qmail 72631 invoked by alias); 26 Aug 2015 06:41:47 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 72620 invoked by uid 89); 26 Aug 2015 06:41:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: zimbra.cs.ucla.edu From: Paul Eggert <eggert@cs.ucla.edu> To: libc-alpha@sourceware.org Cc: Paul Eggert <eggert@cs.ucla.edu> Subject: [PATCH] Fix memory leak in printf_positional Date: Tue, 25 Aug 2015 23:41:35 -0700 Message-Id: <1440571295-20230-1-git-send-email-eggert@cs.ucla.edu> |
Commit Message
Paul Eggert
Aug. 26, 2015, 6:41 a.m. UTC
* stdio-common/vfprintf.c (printf_positional): Free temporary data allocated via malloc. --- ChangeLog | 4 ++++ stdio-common/vfprintf.c | 2 ++ 2 files changed, 6 insertions(+)
Comments
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.
diff --git a/ChangeLog b/ChangeLog index 4ee7c94..bd6d027 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2015-08-25 Paul Eggert <eggert@cs.ucla.edu> + Fix memory leak in printf_positional + * stdio-common/vfprintf.c (printf_positional): + Free temporary data allocated via malloc. + Fix memory leak in _nl_load_domain * intl/loadmsgcat.c (_nl_load_domain): Free data after a read failure. diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c index 0592e70..9d5406a 100644 --- a/stdio-common/vfprintf.c +++ b/stdio-common/vfprintf.c @@ -2091,6 +2091,8 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format, - specs[nspecs_done].end_of_fmt); } all_done: + if (__glibc_unlikely (args_malloced != NULL)) + free (args_malloced); if (__glibc_unlikely (workstart != NULL)) free (workstart); return done;