* Recursion in as_info_where @ 2023-02-01 6:36 Alan Modra 2023-02-01 9:24 ` Jan Beulich 0 siblings, 1 reply; 5+ messages in thread From: Alan Modra @ 2023-02-01 6:36 UTC (permalink / raw) To: binutils [-- Attachment #1: Type: text/plain, Size: 706 bytes --] This function has a gas_assert, ie. possible call to as_abort, which calls as_report_context, which calls as_info_where. Attached fuzzer testcase managed to trigger a stack overflow. * messages.c (as_info_where): Don't gas_assert. diff --git a/gas/messages.c b/gas/messages.c index 0db075d779c..7c018acf69f 100644 --- a/gas/messages.c +++ b/gas/messages.c @@ -141,8 +141,6 @@ as_info_where (const char *file, unsigned int line, unsigned int indent, va_list args; char buffer[2000]; - gas_assert (file != NULL && line > 0 && indent <= INT_MAX); - va_start (args, format); vsnprintf (buffer, sizeof (buffer), format, args); va_end (args); -- Alan Modra Australia Development Lab, IBM [-- Attachment #2: clusterfuzz-testcase-minimized-fuzz_as-5392730034667520 --] [-- Type: application/octet-stream, Size: 26 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Recursion in as_info_where 2023-02-01 6:36 Recursion in as_info_where Alan Modra @ 2023-02-01 9:24 ` Jan Beulich 2023-02-01 11:59 ` Alan Modra 0 siblings, 1 reply; 5+ messages in thread From: Jan Beulich @ 2023-02-01 9:24 UTC (permalink / raw) To: Alan Modra; +Cc: binutils On 01.02.2023 07:36, Alan Modra via Binutils wrote: > This function has a gas_assert, ie. possible call to as_abort, which > calls as_report_context, which calls as_info_where. Attached fuzzer > testcase managed to trigger a stack overflow. > > * messages.c (as_info_where): Don't gas_assert. > > diff --git a/gas/messages.c b/gas/messages.c > index 0db075d779c..7c018acf69f 100644 > --- a/gas/messages.c > +++ b/gas/messages.c > @@ -141,8 +141,6 @@ as_info_where (const char *file, unsigned int line, unsigned int indent, > va_list args; > char buffer[2000]; > > - gas_assert (file != NULL && line > 0 && indent <= INT_MAX); If this go in the way, isn't it that the assertion actually triggered? In which case shouldn't the cause for it triggering be addressed instead, to avoid subsequent knock-on damage (e.g. from de-referencing "file"? (I may want to play with the testcase a little myself.) Jan ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Recursion in as_info_where 2023-02-01 9:24 ` Jan Beulich @ 2023-02-01 11:59 ` Alan Modra 2023-02-03 13:44 ` Jan Beulich 0 siblings, 1 reply; 5+ messages in thread From: Alan Modra @ 2023-02-01 11:59 UTC (permalink / raw) To: Jan Beulich; +Cc: binutils On Wed, Feb 01, 2023 at 10:24:25AM +0100, Jan Beulich wrote: > On 01.02.2023 07:36, Alan Modra via Binutils wrote: > > This function has a gas_assert, ie. possible call to as_abort, which > > calls as_report_context, which calls as_info_where. Attached fuzzer > > testcase managed to trigger a stack overflow. > > > > * messages.c (as_info_where): Don't gas_assert. > > > > diff --git a/gas/messages.c b/gas/messages.c > > index 0db075d779c..7c018acf69f 100644 > > --- a/gas/messages.c > > +++ b/gas/messages.c > > @@ -141,8 +141,6 @@ as_info_where (const char *file, unsigned int line, unsigned int indent, > > va_list args; > > char buffer[2000]; > > > > - gas_assert (file != NULL && line > 0 && indent <= INT_MAX); > > If this go in the way, isn't it that the assertion actually triggered? > In which case shouldn't the cause for it triggering be addressed > instead, to avoid subsequent knock-on damage (e.g. from de-referencing > "file"? (I may want to play with the testcase a little myself.) The testcase is really weird, something that no programmer would ever write. It failed the assert with line == 0. I'll let you discover the horrible "# line file" with embedded \0 that gets you there. :-) I'm not motivated to fix that sort of insanity. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Recursion in as_info_where 2023-02-01 11:59 ` Alan Modra @ 2023-02-03 13:44 ` Jan Beulich 2023-02-06 1:41 ` Alan Modra 0 siblings, 1 reply; 5+ messages in thread From: Jan Beulich @ 2023-02-03 13:44 UTC (permalink / raw) To: Alan Modra; +Cc: binutils On 01.02.2023 12:59, Alan Modra wrote: > On Wed, Feb 01, 2023 at 10:24:25AM +0100, Jan Beulich wrote: >> On 01.02.2023 07:36, Alan Modra via Binutils wrote: >>> This function has a gas_assert, ie. possible call to as_abort, which >>> calls as_report_context, which calls as_info_where. Attached fuzzer >>> testcase managed to trigger a stack overflow. >>> >>> * messages.c (as_info_where): Don't gas_assert. >>> >>> diff --git a/gas/messages.c b/gas/messages.c >>> index 0db075d779c..7c018acf69f 100644 >>> --- a/gas/messages.c >>> +++ b/gas/messages.c >>> @@ -141,8 +141,6 @@ as_info_where (const char *file, unsigned int line, unsigned int indent, >>> va_list args; >>> char buffer[2000]; >>> >>> - gas_assert (file != NULL && line > 0 && indent <= INT_MAX); >> >> If this go in the way, isn't it that the assertion actually triggered? >> In which case shouldn't the cause for it triggering be addressed >> instead, to avoid subsequent knock-on damage (e.g. from de-referencing >> "file"? (I may want to play with the testcase a little myself.) > > The testcase is really weird, something that no programmer would ever > write. It failed the assert with line == 0. I'll let you discover > the horrible "# line file" with embedded \0 that gets you there. :-) Sure. That's an interaction bug between read_a_source_file()'s bumping of line numbers and s_linefile() trying to compensate. I have a fix for that, but I'll want to give it wider testing before posting. As to the assertion here, I'd prefer if we would keep it, and do e.g. the below instead. Thoughts? Jan --- a/gas/messages.c +++ b/gas/messages.c @@ -140,6 +140,12 @@ as_info_where (const char *file, unsigne { va_list args; char buffer[2000]; + static bool active; + + /* We can come back here if e.g. the assertion below triggers. */ + if (active) + return; + active = true; gas_assert (file != NULL && line > 0 && indent <= INT_MAX); @@ -148,6 +154,8 @@ as_info_where (const char *file, unsigne va_end (args); fprintf (stderr, "%s:%u: %*s%s%s\n", file, line, (int)indent, "", _("Info: "), buffer); + + active = false; } /* Send to stderr a string as a warning, and locate warning ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Recursion in as_info_where 2023-02-03 13:44 ` Jan Beulich @ 2023-02-06 1:41 ` Alan Modra 0 siblings, 0 replies; 5+ messages in thread From: Alan Modra @ 2023-02-06 1:41 UTC (permalink / raw) To: Jan Beulich; +Cc: binutils On Fri, Feb 03, 2023 at 02:44:06PM +0100, Jan Beulich wrote: > On 01.02.2023 12:59, Alan Modra wrote: > > On Wed, Feb 01, 2023 at 10:24:25AM +0100, Jan Beulich wrote: > >> On 01.02.2023 07:36, Alan Modra via Binutils wrote: > >>> This function has a gas_assert, ie. possible call to as_abort, which > >>> calls as_report_context, which calls as_info_where. Attached fuzzer > >>> testcase managed to trigger a stack overflow. > >>> > >>> * messages.c (as_info_where): Don't gas_assert. > >>> > >>> diff --git a/gas/messages.c b/gas/messages.c > >>> index 0db075d779c..7c018acf69f 100644 > >>> --- a/gas/messages.c > >>> +++ b/gas/messages.c > >>> @@ -141,8 +141,6 @@ as_info_where (const char *file, unsigned int line, unsigned int indent, > >>> va_list args; > >>> char buffer[2000]; > >>> > >>> - gas_assert (file != NULL && line > 0 && indent <= INT_MAX); > >> > >> If this go in the way, isn't it that the assertion actually triggered? > >> In which case shouldn't the cause for it triggering be addressed > >> instead, to avoid subsequent knock-on damage (e.g. from de-referencing > >> "file"? (I may want to play with the testcase a little myself.) > > > > The testcase is really weird, something that no programmer would ever > > write. It failed the assert with line == 0. I'll let you discover > > the horrible "# line file" with embedded \0 that gets you there. :-) > > Sure. That's an interaction bug between read_a_source_file()'s bumping > of line numbers and s_linefile() trying to compensate. I have a fix for > that, but I'll want to give it wider testing before posting. > > As to the assertion here, I'd prefer if we would keep it, and do e.g. > the below instead. Thoughts? I think it is bad practice for a low-level error handling function to assert. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-02-06 1:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-01 6:36 Recursion in as_info_where Alan Modra 2023-02-01 9:24 ` Jan Beulich 2023-02-01 11:59 ` Alan Modra 2023-02-03 13:44 ` Jan Beulich 2023-02-06 1:41 ` Alan Modra
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).