* [PATCH] Fix PR preprocessor/58893 access to uninitialized memory @ 2014-09-26 12:16 Bernd Edlinger 2014-09-26 12:19 ` Marek Polacek 2014-09-26 12:21 ` FW: " Bernd Edlinger 0 siblings, 2 replies; 8+ messages in thread From: Bernd Edlinger @ 2014-09-26 12:16 UTC (permalink / raw) To: gcc-patches; +Cc: Jeff Law, Joseph S. Myers Hi, this patch fixes PR58893, which is an access to uninitialized memory, which may or may not crash in linemap_resolve_location, or just print error messages with bogus location. When the first -include file is processed we have the case, where pfile->cur_token == pfile->cur_run->base, this is directly called by the front end. However in the case of the second -include file, this is called from _cpp_lex_token -> _cpp_get_fresh_line -> cpp_push_include, with pfile->cur_token != pfile->cur_run->base, and pfile->cur_token[-1].src_loc and token not (yet) initialized. The problem is, when the include file cannot be found, we need src_loc to be initialized to some safe value: 0 means UNKNOWN_LOCATION. Regarding the hunk in cpp_diagnostic, which is not directly involved in this bug, but it is still obviously wrong: The line "src_loc = pfile->cur_run->prev->limit->src_loc" is probably unreachable, but will crash it is ever executed. see: _cpp_init_tokenrun (tokenrun *run, unsigned int count) { run->base = XNEWVEC (cpp_token, count); run->limit = run->base + count; run->next = NULL; } so, limit points at the end of the run. Boot-Strapped and Regression-tested on x86_64-linux-gnu Ok for trunk? Thanks Bernd. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR preprocessor/58893 access to uninitialized memory 2014-09-26 12:16 [PATCH] Fix PR preprocessor/58893 access to uninitialized memory Bernd Edlinger @ 2014-09-26 12:19 ` Marek Polacek 2014-09-26 12:21 ` FW: " Bernd Edlinger 1 sibling, 0 replies; 8+ messages in thread From: Marek Polacek @ 2014-09-26 12:19 UTC (permalink / raw) To: Bernd Edlinger; +Cc: gcc-patches, Jeff Law, Joseph S. Myers On Fri, Sep 26, 2014 at 02:16:05PM +0200, Bernd Edlinger wrote: > Boot-Strapped and Regression-tested on x86_64-linux-gnu > Ok for trunk? -ENOPATCH. Marek ^ permalink raw reply [flat|nested] 8+ messages in thread
* FW: [PATCH] Fix PR preprocessor/58893 access to uninitialized memory 2014-09-26 12:16 [PATCH] Fix PR preprocessor/58893 access to uninitialized memory Bernd Edlinger 2014-09-26 12:19 ` Marek Polacek @ 2014-09-26 12:21 ` Bernd Edlinger 2014-09-26 18:48 ` Jeff Law 1 sibling, 1 reply; 8+ messages in thread From: Bernd Edlinger @ 2014-09-26 12:21 UTC (permalink / raw) To: gcc-patches; +Cc: Jeff Law, Joseph S. Myers [-- Attachment #1: Type: text/plain, Size: 1398 bytes --] Aehm, sorry., again, with patch files. > > Hi, > > this patch fixes PR58893, which is an access to uninitialized memory, which may or may not crash in > linemap_resolve_location, or just print error messages with bogus location. > > When the first -include file is processed we have the case, where > pfile->cur_token == pfile->cur_run->base, this is directly called > by the front end. However in the case of the second -include file, > this is called from _cpp_lex_token -> _cpp_get_fresh_line -> > cpp_push_include, with pfile->cur_token != pfile->cur_run->base, > and pfile->cur_token[-1].src_loc and token not (yet) initialized. > The problem is, when the include file cannot be found, we need > src_loc to be initialized to some safe value: 0 means UNKNOWN_LOCATION. > > Regarding the hunk in cpp_diagnostic, which is not directly involved > in this bug, but it is still obviously wrong: > > The line "src_loc = pfile->cur_run->prev->limit->src_loc" > is probably unreachable, but will crash it is ever executed. > > see: > > _cpp_init_tokenrun (tokenrun *run, unsigned int count) > { > run->base = XNEWVEC (cpp_token, count); > run->limit = run->base + count; > run->next = NULL; > } > > so, limit points at the end of the run. > > > Boot-Strapped and Regression-tested on x86_64-linux-gnu > Ok for trunk? > > > Thanks > Bernd. > [-- Attachment #2: changelog-pr58893.txt --] [-- Type: text/plain, Size: 214 bytes --] 2014-09-26 Bernd Edlinger <bernd.edlinger@hotmail.de> PR preprocessor/58893 * errors.c (cpp_diagnostic): Fix possible out of bounds access. * files.c (_cpp_stack_include): Initialize src_loc for IT_CMDLINE. [-- Attachment #3: patch-pr58893.diff --] [-- Type: application/octet-stream, Size: 852 bytes --] --- libcpp/errors.c 2014-01-02 23:24:45.000000000 +0100 +++ libcpp/errors.c 2014-09-24 10:30:33.708048505 +0200 @@ -48,10 +48,7 @@ cpp_diagnostic (cpp_reader * pfile, int current run -- that is invalid. */ else if (pfile->cur_token == pfile->cur_run->base) { - if (pfile->cur_run->prev != NULL) - src_loc = pfile->cur_run->prev->limit->src_loc; - else - src_loc = 0; + src_loc = 0; } else { --- libcpp/files.c 2014-05-21 20:54:12.000000000 +0200 +++ libcpp/files.c 2014-09-24 10:35:47.191117490 +0200 @@ -991,6 +991,9 @@ _cpp_stack_include (cpp_reader *pfile, c _cpp_file *file; bool stacked; + if (type == IT_CMDLINE && pfile->cur_token != pfile->cur_run->base) + pfile->cur_token[-1].src_loc = 0; + dir = search_path_head (pfile, fname, angle_brackets, type); if (!dir) return false; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: FW: [PATCH] Fix PR preprocessor/58893 access to uninitialized memory 2014-09-26 12:21 ` FW: " Bernd Edlinger @ 2014-09-26 18:48 ` Jeff Law [not found] ` <DUB118-W46D6B67D3766B4DE9B85D7E4BC0@phx.gbl> 0 siblings, 1 reply; 8+ messages in thread From: Jeff Law @ 2014-09-26 18:48 UTC (permalink / raw) To: Bernd Edlinger, gcc-patches; +Cc: Joseph S. Myers On 09/26/14 06:21, Bernd Edlinger wrote: >> > >> >Hi, >> > >> >this patch fixes PR58893, which is an access to uninitialized memory, which may or may not crash in >> >linemap_resolve_location, or just print error messages with bogus location. >> > >> >When the first -include file is processed we have the case, where >> >pfile->cur_token == pfile->cur_run->base, this is directly called >> >by the front end. However in the case of the second -include file, >> >this is called from _cpp_lex_token -> _cpp_get_fresh_line -> >> >cpp_push_include, with pfile->cur_token != pfile->cur_run->base, >> >and pfile->cur_token[-1].src_loc and token not (yet) initialized. >> >The problem is, when the include file cannot be found, we need >> >src_loc to be initialized to some safe value: 0 means UNKNOWN_LOCATION. >> > >> >Regarding the hunk in cpp_diagnostic, which is not directly involved >> >in this bug, but it is still obviously wrong: >> > >> >The line "src_loc = pfile->cur_run->prev->limit->src_loc" >> >is probably unreachable, but will crash it is ever executed. >> > >> >see: >> > >> >_cpp_init_tokenrun (tokenrun *run, unsigned int count) >> >{ >> >run->base = XNEWVEC (cpp_token, count); >> >run->limit = run->base + count; >> >run->next = NULL; >> >} >> > >> >so, limit points at the end of the run. >> > >> > >> >Boot-Strapped and Regression-tested on x86_64-linux-gnu >> >Ok for trunk? >> > >> > >> >Thanks >> >Bernd. >> > > > > > changelog-pr58893.txt > > > 2014-09-26 Bernd Edlinger<bernd.edlinger@hotmail.de> > > PR preprocessor/58893 > * errors.c (cpp_diagnostic): Fix possible out of bounds access. > * files.c (_cpp_stack_include): Initialize src_loc for IT_CMDLINE. > > > patch-pr58893.diff > > > --- libcpp/errors.c 2014-01-02 23:24:45.000000000 +0100 > +++ libcpp/errors.c 2014-09-24 10:30:33.708048505 +0200 > @@ -48,10 +48,7 @@ cpp_diagnostic (cpp_reader * pfile, int > current run -- that is invalid. */ > else if (pfile->cur_token == pfile->cur_run->base) > { > - if (pfile->cur_run->prev != NULL) > - src_loc = pfile->cur_run->prev->limit->src_loc; > - else > - src_loc = 0; > + src_loc = 0; > } > else > { > --- libcpp/files.c 2014-05-21 20:54:12.000000000 +0200 > +++ libcpp/files.c 2014-09-24 10:35:47.191117490 +0200 > @@ -991,6 +991,9 @@ _cpp_stack_include (cpp_reader *pfile, c > _cpp_file *file; > bool stacked; > > + if (type == IT_CMDLINE && pfile->cur_token != pfile->cur_run->base) > + pfile->cur_token[-1].src_loc = 0; Comment before this change. Someone not familiar with this code is going to have no idea why these two lines exist. Please try to include a testcase. If you're having trouble reproducing on the trunk, you could use MALLOC_PERTURB per c#8 in the bug report. If there's a way to set environment variables in our testing framework that may be a reasonable way to test (if you need to do that, limit testing to linux targets as we'll have a dependency on glibc features). jeff ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <DUB118-W46D6B67D3766B4DE9B85D7E4BC0@phx.gbl>]
* RE: [PATCH] Fix PR preprocessor/58893 access to uninitialized memory [not found] ` <DUB118-W46D6B67D3766B4DE9B85D7E4BC0@phx.gbl> @ 2014-09-27 9:53 ` Bernd Edlinger 2014-09-30 4:41 ` Jeff Law 0 siblings, 1 reply; 8+ messages in thread From: Bernd Edlinger @ 2014-09-27 9:53 UTC (permalink / raw) To: gcc-patches [-- Attachment #1: Type: text/plain, Size: 6490 bytes --] Hmm, original massage bounced, resent, without html. ________________________________ > From: bernd.edlinger@hotmail.de > To: law@redhat.com; gcc-patches@gcc.gnu.org > CC: joseph@codesourcery.com > Subject: RE: [PATCH] Fix PR preprocessor/58893 access to uninitialized memory > Date: Sat, 27 Sep 2014 11:42:29 +0200 > > > > On Fri, 26 Sep 2014 12:48:44, Jeff Law wrote: > > > > On 09/26/14 06:21, Bernd Edlinger wrote: > >>>> > >>>>Hi, > >>>> > >>>>this patch fixes PR58893, which is an access to uninitialized > memory, which may or may not crash in > >>>>linemap_resolve_location, or just print error messages with bogus > location. > >>>> > >>>>When the first -include file is processed we have the case, where > >>>>pfile->cur_token == pfile->cur_run->base, this is directly called > >>>>by the front end. However in the case of the second -include file, > >>>>this is called from _cpp_lex_token -> _cpp_get_fresh_line -> > >>>>cpp_push_include, with pfile->cur_token != pfile->cur_run->base, > >>>>and pfile->cur_token[-1].src_loc and token not (yet) initialized. > >>>>The problem is, when the include file cannot be found, we need > >>>>src_loc to be initialized to some safe value: 0 means UNKNOWN_LOCATION. > >>>> > >>>>Regarding the hunk in cpp_diagnostic, which is not directly involved > >>>>in this bug, but it is still obviously wrong: > >>>> > >>>>The line "src_loc = pfile->cur_run->prev->limit->src_loc" > >>>>is probably unreachable, but will crash it is ever executed. > >>>> > >>>>see: > >>>> > >>>>_cpp_init_tokenrun (tokenrun *run, unsigned int count) > >>>>{ > >>>>run->base = XNEWVEC (cpp_token, count); > >>>>run->limit = run->base + count; > >>>>run->next = NULL; > >>>>} > >>>> > >>>>so, limit points at the end of the run. > >>>> > >>>> > >>>>Boot-Strapped and Regression-tested on x86_64-linux-gnu > >>>>Ok for trunk? > >>>> > >>>> > >>>>Thanks > >>>>Bernd. > >>>> > >> > >> > >> > >> changelog-pr58893.txt > >> > >> > >> 2014-09-26 Bernd Edlinger<bernd.edlinger@hotmail.de> > >> > >> PR preprocessor/58893 > >> * errors.c (cpp_diagnostic): Fix possible out of bounds access. > >> * files.c (_cpp_stack_include): Initialize src_loc for IT_CMDLINE. > >> > >> > >> patch-pr58893.diff > >> > >> > >> --- libcpp/errors.c 2014-01-02 23:24:45.000000000 +0100 > >> +++ libcpp/errors.c 2014-09-24 10:30:33.708048505 +0200 > >> @@ -48,10 +48,7 @@ cpp_diagnostic (cpp_reader * pfile, int > >> current run -- that is invalid. */ > >> else if (pfile->cur_token == pfile->cur_run->base) > >> { > >> - if (pfile->cur_run->prev != NULL) > >> - src_loc = pfile->cur_run->prev->limit->src_loc; > >> - else > >> - src_loc = 0; > >> + src_loc = 0; > >> } > >> else > >> { > >> --- libcpp/files.c 2014-05-21 20:54:12.000000000 +0200 > >> +++ libcpp/files.c 2014-09-24 10:35:47.191117490 +0200 > >> @@ -991,6 +991,9 @@ _cpp_stack_include (cpp_reader *pfile, c > >> _cpp_file *file; > >> bool stacked; > >> > >> + if (type == IT_CMDLINE && pfile->cur_token != pfile->cur_run->base) > >> + pfile->cur_token[-1].src_loc = 0; > > Comment before this change. Someone not familiar with this code is > > going to have no idea why these two lines exist. > > > > Ok, I added a comment now, do you like it? > > > Please try to include a testcase. If you're having trouble reproducing > > on the trunk, you could use MALLOC_PERTURB per c#8 in the bug report. > > If there's a way to set environment variables in our testing framework > > that may be a reasonable way to test (if you need to do that, limit > > testing to linux targets as we'll have a dependency on glibc features). > > > > For whatever reason, the first -include must end with a pragma > as in the PR, and MALLOC_PERTURB_ must be set to something. > Then we get an ICE, otherwise we get an error message without line number. > I tried to make this a valid test case, but that might be less trivial than > it looks at first sight. > > I tried to set MALLOC_PERTURB_=123 globally, like this: > > MALLOC_PERTURB_=123 make -k check > > but then this happened: > > .... > WARNING: program timed out. > FAIL: gcc.c-torture/unsorted/dump-noaddr.c, -O3 -fomit-frame-pointer > -funroll-all-loops -finline-functions -dumpbase dump1/dump-noaddr.c > -DMASK=1 -x c --param ggc-min-heapsize=1 -fdump-ipa-all -fdump-rtl-all > -fdump-tree-all -fdump-noaddr > FAIL: gcc.c-torture/unsorted/dump-noaddr.c.000i.cgraph, -O3 > -fomit-frame-pointer -funroll-all-loops -finline-functions comparison > FAIL: gcc.c-torture/unsorted/dump-noaddr.c.003t.original, -O3 > -fomit-frame-pointer -funroll-all-loops -finline-functions comparison > FAIL: gcc.c-torture/unsorted/dump-noaddr.c.032t.profile_estimate, -O3 > -fomit-frame-pointer -funroll-all-loops -finline-functions comparison > FAIL: gcc.c-torture/unsorted/dump-noaddr.c.253t.statistics, -O3 > -fomit-frame-pointer -funroll-all-loops -finline-functions comparison > WARNING: program timed out. > FAIL: gcc.c-torture/unsorted/dump-noaddr.c, -O3 -g -dumpbase > dump1/dump-noaddr.c -DMASK=1 -x c --param ggc-min-heapsize=1 > -fdump-ipa-all -fdump-rtl-all -fdump-tree-all -fdump-noaddr > FAIL: gcc.c-torture/unsorted/dump-noaddr.c.000i.cgraph, -O3 -g comparison > FAIL: gcc.c-torture/unsorted/dump-noaddr.c.003t.original, -O3 -g comparison > FAIL: gcc.c-torture/unsorted/dump-noaddr.c.032t.profile_estimate, -O3 > -g comparison > FAIL: gcc.c-torture/unsorted/dump-noaddr.c.253t.statistics, -O3 -g > comparison > ^Cgot a INT signal, interrupted by user > > Well I am afraid this test case alone takes hours, and would disrupt > the whole test suite, > so currently I think it would be the right thing to set MALLOC_PERTURB_=123 > globally in the test suite, but this looks not like a small step for > one man.... > > Any Ideas what is wrong with that test case? > > > Well, I added a test case, but it does not reliably fail without the > patch, because setting > MALLOC_PERTURB_ causes too much trouble at this time. > > I would propose to set MALLOC_PERTURB_ globally at a later time. > > Boot-Strapped & Regression-Tested on x86_64-linux-gnu. > Ok for trunk? > > > Thanks > Bernd. > > > jeff > > [-- Attachment #2: changelog-pr58893.txt --] [-- Type: text/plain, Size: 385 bytes --] libcpp: 2014-09-27 Bernd Edlinger <bernd.edlinger@hotmail.de> PR preprocessor/58893 * errors.c (cpp_diagnostic): Fix possible out of bounds access. * files.c (_cpp_stack_include): Initialize src_loc for IT_CMDLINE. testsuite: 2014-09-27 Bernd Edlinger <bernd.edlinger@hotmail.de> PR preprocessor/58893 * gcc.dg/pr58893.c: New test case. * gcc.dg/pr58893-0.h: New include. [-- Attachment #3: patch-pr58893.diff --] [-- Type: application/octet-stream, Size: 2360 bytes --] Index: gcc/testsuite/gcc.dg/pr58893-0.h =================================================================== --- gcc/testsuite/gcc.dg/pr58893-0.h (Revision 0) +++ gcc/testsuite/gcc.dg/pr58893-0.h (Arbeitskopie) @@ -0,0 +1 @@ +#pragma GCC visibility push(hidden) Index: gcc/testsuite/gcc.dg/pr58893.c =================================================================== --- gcc/testsuite/gcc.dg/pr58893.c (Revision 0) +++ gcc/testsuite/gcc.dg/pr58893.c (Arbeitskopie) @@ -0,0 +1,5 @@ +/* PR preprocessor/58893 */ +/* { dg-do compile } */ +/* { dg-options "-include pr58893-0.h -include pr58893-1.h -I${srcdir}/gcc.dg" } */ +/* { dg-error "pr58893-1.h: No such file or directory" "" { target *-*-* } 0 } */ +/* { dg-prune-output "compilation terminated" } */ Index: libcpp/errors.c =================================================================== --- libcpp/errors.c (Revision 215644) +++ libcpp/errors.c (Arbeitskopie) @@ -48,10 +48,7 @@ cpp_diagnostic (cpp_reader * pfile, int level, int current run -- that is invalid. */ else if (pfile->cur_token == pfile->cur_run->base) { - if (pfile->cur_run->prev != NULL) - src_loc = pfile->cur_run->prev->limit->src_loc; - else - src_loc = 0; + src_loc = 0; } else { Index: libcpp/files.c =================================================================== --- libcpp/files.c (Revision 215644) +++ libcpp/files.c (Arbeitskopie) @@ -991,6 +991,18 @@ _cpp_stack_include (cpp_reader *pfile, const char _cpp_file *file; bool stacked; + /* For -include command-line flags we have type == IT_CMDLINE. + When the first -include file is processed we have the case, where + pfile->cur_token == pfile->cur_run->base, we are directly called up + by the front end. However in the case of the second -include file, + we are called from _cpp_lex_token -> _cpp_get_fresh_line -> + cpp_push_include, with pfile->cur_token != pfile->cur_run->base, + and pfile->cur_token[-1].src_loc not (yet) initialized. + However, when the include file cannot be found, we need src_loc to + be initialized to some safe value: 0 means UNKNOWN_LOCATION. */ + if (type == IT_CMDLINE && pfile->cur_token != pfile->cur_run->base) + pfile->cur_token[-1].src_loc = 0; + dir = search_path_head (pfile, fname, angle_brackets, type); if (!dir) return false; ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR preprocessor/58893 access to uninitialized memory 2014-09-27 9:53 ` Bernd Edlinger @ 2014-09-30 4:41 ` Jeff Law 2014-09-30 9:01 ` Bernd Edlinger 0 siblings, 1 reply; 8+ messages in thread From: Jeff Law @ 2014-09-30 4:41 UTC (permalink / raw) To: Bernd Edlinger, gcc-patches On 09/27/14 03:53, Bernd Edlinger wrote: >>> Comment before this change. Someone not familiar with this code is >>> going to have no idea why these two lines exist. >>> >> >> Ok, I added a comment now, do you like it? Yes. >> >>> Please try to include a testcase. If you're having trouble reproducing >>> on the trunk, you could use MALLOC_PERTURB per c#8 in the bug report. >>> If there's a way to set environment variables in our testing framework >>> that may be a reasonable way to test (if you need to do that, limit >>> testing to linux targets as we'll have a dependency on glibc features). >>> >> >> For whatever reason, the first -include must end with a pragma >> as in the PR, and MALLOC_PERTURB_ must be set to something. >> Then we get an ICE, otherwise we get an error message without line number. >> I tried to make this a valid test case, but that might be less trivial than >> it looks at first sight. >> >> I tried to set MALLOC_PERTURB_=123 globally, like this: >> >> MALLOC_PERTURB_=123 make -k check >> >> but then this happened: Sigh. Yea, I guess if we're hitting the allocator insanely hard, scrubbing memory might turn out to slow things down in a significant way. Or it may simply be the case that we're using free'd memory in some way and with the MALLOC_PERTURB changes we're in an infinite loop in the dumping code or something similar. >> >> >> Well, I added a test case, but it does not reliably fail without the >> patch, because setting >> MALLOC_PERTURB_ causes too much trouble at this time. >> >> I would propose to set MALLOC_PERTURB_ globally at a later time. Sorry, just to be clear, I wasn't suggesting to set it globally, but just for the duration of this test as a potentially easier way to trigger the failure. However, it may make sense to do that at some point. I also think that Jakub bootstraps and runs the regression suite with valgrind late in the release cycle, which would catch this problem if it raises its head again. >> >> Boot-Strapped & Regression-Tested on x86_64-linux-gnu. >> Ok for trunk? Yes, this is OK for the trunk. jeff ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] Fix PR preprocessor/58893 access to uninitialized memory 2014-09-30 4:41 ` Jeff Law @ 2014-09-30 9:01 ` Bernd Edlinger 2014-09-30 16:38 ` Jeff Law 0 siblings, 1 reply; 8+ messages in thread From: Bernd Edlinger @ 2014-09-30 9:01 UTC (permalink / raw) To: Jeff Law, gcc-patches Hi Jeff, On Mon, 29 Sep 2014 22:40:58, Jeff Law wrote: > > On 09/27/14 03:53, Bernd Edlinger wrote: >>>> Comment before this change. Someone not familiar with this code is >>>> going to have no idea why these two lines exist. >>>> >>> >>> Ok, I added a comment now, do you like it? > Yes. > > >>> >>>> Please try to include a testcase. If you're having trouble reproducing >>>> on the trunk, you could use MALLOC_PERTURB per c#8 in the bug report. >>>> If there's a way to set environment variables in our testing framework >>>> that may be a reasonable way to test (if you need to do that, limit >>>> testing to linux targets as we'll have a dependency on glibc features). >>>> >>> >>> For whatever reason, the first -include must end with a pragma >>> as in the PR, and MALLOC_PERTURB_ must be set to something. >>> Then we get an ICE, otherwise we get an error message without line number. >>> I tried to make this a valid test case, but that might be less trivial than >>> it looks at first sight. > >>> >>> I tried to set MALLOC_PERTURB_=123 globally, like this: >>> >>> MALLOC_PERTURB_=123 make -k check >>> >>> but then this happened: > Sigh. Yea, I guess if we're hitting the allocator insanely hard, > scrubbing memory might turn out to slow things down in a significant > way. Or it may simply be the case that we're using free'd memory in > some way and with the MALLOC_PERTURB changes we're in an infinite loop > in the dumping code or something similar. > Yeah, that is an interesting thing. I debugged that, and it turns out, that this is just incredibly slow. It seems to be in the macro expansion of this construct: #define t16(x) x x x x x x x x x x x x x x x x #define M (sizeof (t16(t16(t16(t16(t16(" ")))))) - 1) libcpp is calling realloc 1.000.000 times for this, resizing the memory by just one byte at a time. And the worst case of realloc is O(n), so in the worst case realloc would have to copy 1/2 * 1.000.000^2 bytes = 500 GB of memory. With this little change in libcpp, the test suite passed, without any further regressions: --- libcpp/charset.c.jj 2014-08-19 07:34:31.000000000 +0200 +++ libcpp/charset.c 2014-09-30 10:45:26.676954120 +0200 @@ -537,6 +537,7 @@ convert_no_conversion (iconv_t cd ATTRIB if (to->len + flen> to->asize) { to->asize = to->len + flen; + to->asize *= 2; to->text = XRESIZEVEC (uchar, to->text, to->asize); } memcpy (to->text + to->len, from, flen); I will prepare a patch for that later. Interestingly, if I define MALLOC_CHECK_=3 _and_ MALLOC_PERTURB_ this test passes, even without the above change, but the test case gfortran.dg/realloc_on_assign_5.f03 fails in this configuration, which is a known bug: PR 47674. However it passes when only MALLOC_PERTURB_ is defined. Weird... > >>> >>> >>> Well, I added a test case, but it does not reliably fail without the >>> patch, because setting >>> MALLOC_PERTURB_ causes too much trouble at this time. >>> >>> I would propose to set MALLOC_PERTURB_ globally at a later time. > Sorry, just to be clear, I wasn't suggesting to set it globally, but > just for the duration of this test as a potentially easier way to > trigger the failure. > > However, it may make sense to do that at some point. I also think that > Jakub bootstraps and runs the regression suite with valgrind late in the > release cycle, which would catch this problem if it raises its head again. > >>> >>> Boot-Strapped & Regression-Tested on x86_64-linux-gnu. >>> Ok for trunk? > Yes, this is OK for the trunk. > Thanks! Bernd. > jeff > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Fix PR preprocessor/58893 access to uninitialized memory 2014-09-30 9:01 ` Bernd Edlinger @ 2014-09-30 16:38 ` Jeff Law 0 siblings, 0 replies; 8+ messages in thread From: Jeff Law @ 2014-09-30 16:38 UTC (permalink / raw) To: Bernd Edlinger, gcc-patches On 09/30/14 03:01, Bernd Edlinger wrote: >> Sigh. Yea, I guess if we're hitting the allocator insanely hard, >> scrubbing memory might turn out to slow things down in a significant >> way. Or it may simply be the case that we're using free'd memory in >> some way and with the MALLOC_PERTURB changes we're in an infinite loop >> in the dumping code or something similar. >> > > Yeah, that is an interesting thing. > I debugged that, and it turns out, that this is just incredibly slow. > It seems to be in the macro expansion of this construct: > > #define t16(x) x x x x x x x x x x x x x x x x > #define M (sizeof (t16(t16(t16(t16(t16(" ")))))) - 1) > > libcpp is calling realloc 1.000.000 times for this, resizing > the memory by just one byte at a time. And the worst case of > realloc is O(n), so in the worst case realloc would have > to copy 1/2 * 1.000.000^2 bytes = 500 GB of memory. > > With this little change in libcpp, the test suite passed, without any > further regressions: > > --- libcpp/charset.c.jj 2014-08-19 07:34:31.000000000 +0200 > +++ libcpp/charset.c 2014-09-30 10:45:26.676954120 +0200 > @@ -537,6 +537,7 @@ convert_no_conversion (iconv_t cd ATTRIB > if (to->len + flen> to->asize) > { > to->asize = to->len + flen; > + to->asize *= 2; > to->text = XRESIZEVEC (uchar, to->text, to->asize); > } > memcpy (to->text + to->len, from, flen); > > I will prepare a patch for that later. Thanks for digging into this. We usually try to throttle this growth a little. Something like this would be consistent with other cases in GCC: to->asize += to->asize / 4; > > Interestingly, if I define MALLOC_CHECK_=3 _and_ MALLOC_PERTURB_ > this test passes, even without the above change, > but the test case > gfortran.dg/realloc_on_assign_5.f03 fails in this configuration, > which is a known bug: PR 47674. However it passes when only MALLOC_PERTURB_ > is defined. > > Weird... Yea, but that's par for the course when dealing with memory errors. Jeff ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-09-30 16:38 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-26 12:16 [PATCH] Fix PR preprocessor/58893 access to uninitialized memory Bernd Edlinger 2014-09-26 12:19 ` Marek Polacek 2014-09-26 12:21 ` FW: " Bernd Edlinger 2014-09-26 18:48 ` Jeff Law [not found] ` <DUB118-W46D6B67D3766B4DE9B85D7E4BC0@phx.gbl> 2014-09-27 9:53 ` Bernd Edlinger 2014-09-30 4:41 ` Jeff Law 2014-09-30 9:01 ` Bernd Edlinger 2014-09-30 16:38 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).