* Re: [Bug-readline] heap-buffer-overflow in update_line [not found] ` <d29fbf1f-d33e-77f6-90ed-0eb3779a027c@case.edu> @ 2019-05-17 14:59 ` Tom de Vries 2019-05-17 15:34 ` Chet Ramey 2019-05-20 20:14 ` Chet Ramey 0 siblings, 2 replies; 7+ messages in thread From: Tom de Vries @ 2019-05-17 14:59 UTC (permalink / raw) To: chet.ramey, bug-readline; +Cc: gdb-patches, Pedro Alves On 16-05-19 22:50, Chet Ramey wrote: > On 5/8/19 4:10 PM, Tom de Vries wrote: >> Hi, >> >> when: >> - building trunk gdb (using the readline sources in the binutils-gdb.git >> repo) on openSUSE 15.0 x86_64-linux with -fsanitize=address, and: >> - running gdb tests with "export ASAN_OPTIONS=detect_leaks=0", >> I run into a heap-buffer-overflow failure for >> gdb.base/utf8-identifiers.exp, reported as PR gdb/24514 - >> "heap-buffer-overflow in update_line for utf8-identifiers.exp" at >> https://sourceware.org/bugzilla/show_bug.cgi?id=24514 . > > [...] > >> which triggers without needing the address sanitizer, like this: >> ... >> $ TERM=dumb gdb -q -ex "set width 0" >> gdb: /home/vries/readline/src/display.c:1393: rl_redisplay: Assertion >> `last_lmargin + (_rl_screenwidth + visible_wrap_offset) <= line_size' >> failed. >> Aborted (core dumped) > > This looks like the same problem as described in > > http://lists.gnu.org/archive/html/bug-readline/2019-03/msg00001.html > > In this case, gdb sets the screen width to 32766, which is obviously > bonkers on a dumb terminal. Gdb should pass -1 to rl_set_screen_size > so readline doesn't override the number of columns on the physical > terminal. > I've tried this: ... diff --git a/gdb/utils.c b/gdb/utils.c index 9686927473..2bfa22055e 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1371,7 +1371,7 @@ set_screen_size (void) if (cols <= 0 || cols > sqrt_int_max) { - cols = sqrt_int_max; + cols = -1; chars_per_line = UINT_MAX; } ... but ran into this test failure in gdb.ada/pp-rec-component.exp: ... (gdb) source /data/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/pp^M<sions/devel/build/gdb/testsuite/outputs/gdb.ada/pp- ^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^Hrec-component/pp-rec-com ^M<tsuite/outputs/gdb.ada/pp-rec-component/pp-rec-comp ^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^Honent.py^M (gdb) FAIL: gdb.ada/pp-rec-component.exp: source pp-rec-component.py ... which is readline doing it's horizontal scrolling mode, which AFAIU is triggered by this condition in rl_redisplay failing: ... if (_rl_horizontal_scroll_mode == 0 && _rl_term_up && *_rl_term_up) ... not because _rl_horizontal_scroll_mode is 1, but because _rl_term_up is NULL (because of TERM=dumb). Note btw that ^H is used here by readline despite the fact that TERM=dumb does not support backspace. I'm not sure if this is a bug, or intentional behaviour. Either way, I'm open for suggestions that make gdb call rl_set_screen_size with legal parameters, and disable features like horizontal scrolling to get unformatted output for the testsuite run. Thanks, - Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug-readline] heap-buffer-overflow in update_line 2019-05-17 14:59 ` [Bug-readline] heap-buffer-overflow in update_line Tom de Vries @ 2019-05-17 15:34 ` Chet Ramey 2019-05-20 20:14 ` Chet Ramey 1 sibling, 0 replies; 7+ messages in thread From: Chet Ramey @ 2019-05-17 15:34 UTC (permalink / raw) To: Tom de Vries, bug-readline; +Cc: chet.ramey, gdb-patches, Pedro Alves On 5/17/19 10:59 AM, Tom de Vries wrote: > > I've tried this: > ... > diff --git a/gdb/utils.c b/gdb/utils.c > index 9686927473..2bfa22055e 100644 > --- a/gdb/utils.c > +++ b/gdb/utils.c > @@ -1371,7 +1371,7 @@ set_screen_size (void) > > if (cols <= 0 || cols > sqrt_int_max) > { > - cols = sqrt_int_max; > + cols = -1; > chars_per_line = UINT_MAX; > } It's not apparent from this patch whether or not gdb uses `cols' for anything besides passing to readline. > ... > but ran into this test failure in gdb.ada/pp-rec-component.exp: > ... > (gdb) source > /data/gdb_versions/devel/build/gdb/testsuite/outputs/gdb.ada/pp^M<sions/devel/build/gdb/testsuite/outputs/gdb.ada/pp- > > ^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^Hrec-component/pp-rec-com > ^M<tsuite/outputs/gdb.ada/pp-rec-component/pp-rec-comp > ^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^H^Honent.py^M > (gdb) FAIL: gdb.ada/pp-rec-component.exp: source pp-rec-component.py > ... > which is readline doing it's horizontal scrolling mode, which AFAIU is > triggered by this condition in rl_redisplay failing: > ... > if (_rl_horizontal_scroll_mode == 0 && _rl_term_up && *_rl_term_up) > ... > not because _rl_horizontal_scroll_mode is 1, but because _rl_term_up is > NULL (because of TERM=dumb). Correct. If the terminal type is unknown or tgetent returns a set of capabilities that doesn't include "up", you're going to get horizontal scrolling. > > Note btw that ^H is used here by readline despite the fact that > TERM=dumb does not support backspace. I'm not sure if this is a bug, or > intentional behaviour. Readline uses what tgetent returns for "le" or defaults to "\b" if tgetent fails. Most dumb terminals support ^H to move the cursor left one position. > > Either way, I'm open for suggestions that make gdb call > rl_set_screen_size with legal parameters, and disable features like > horizontal scrolling to get unformatted output for the testsuite run. You can't specify a dumb terminal and expect to have line wrapping. Chet -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, UTech, CWRU chet@case.edu http://tiswww.cwru.edu/~chet/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug-readline] heap-buffer-overflow in update_line 2019-05-17 14:59 ` [Bug-readline] heap-buffer-overflow in update_line Tom de Vries 2019-05-17 15:34 ` Chet Ramey @ 2019-05-20 20:14 ` Chet Ramey 2019-05-23 7:33 ` Tom de Vries 1 sibling, 1 reply; 7+ messages in thread From: Chet Ramey @ 2019-05-20 20:14 UTC (permalink / raw) To: Tom de Vries, bug-readline; +Cc: chet.ramey, gdb-patches, Pedro Alves On 5/17/19 10:59 AM, Tom de Vries wrote: > Either way, I'm open for suggestions that make gdb call > rl_set_screen_size with legal parameters, and disable features like > horizontal scrolling to get unformatted output for the testsuite run. Here's a patch that will prevent the huge values for the screen width from causing at least one issue with line_size: *** ../readline-8.0-patched/display.c 2018-09-30 21:37:48.000000000 -0400 --- display.c 2019-05-16 16:50:44.000000000 -0400 *************** *** 604,607 **** --- 604,610 ---- register int n; + if (line_size <= _rl_screenwidth) /* XXX - for gdb */ + line_size = _rl_screenwidth + 1; + if (invisible_line == 0) /* initialize it */ { You're still going to have to deal with some horizontal scrolling if the input line gets long enough. -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, UTech, CWRU chet@case.edu http://tiswww.cwru.edu/~chet/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug-readline] heap-buffer-overflow in update_line 2019-05-20 20:14 ` Chet Ramey @ 2019-05-23 7:33 ` Tom de Vries 2019-05-23 12:38 ` Chet Ramey 0 siblings, 1 reply; 7+ messages in thread From: Tom de Vries @ 2019-05-23 7:33 UTC (permalink / raw) To: chet.ramey, bug-readline; +Cc: gdb-patches, Pedro Alves On 20-05-19 22:14, Chet Ramey wrote: > On 5/17/19 10:59 AM, Tom de Vries wrote: > >> Either way, I'm open for suggestions that make gdb call >> rl_set_screen_size with legal parameters, and disable features like >> horizontal scrolling to get unformatted output for the testsuite run. > > Here's a patch that will prevent the huge values for the screen width from > causing at least one issue with line_size: > > *** ../readline-8.0-patched/display.c 2018-09-30 21:37:48.000000000 -0400 > --- display.c 2019-05-16 16:50:44.000000000 -0400 > *************** > *** 604,607 **** > --- 604,610 ---- > register int n; > > + if (line_size <= _rl_screenwidth) /* XXX - for gdb */ > + line_size = _rl_screenwidth + 1; > + > if (invisible_line == 0) /* initialize it */ > { > > You're still going to have to deal with some horizontal scrolling if the > input line gets long enough. > Hi Chet, thanks for the patch. I've tried it out (together with the assert mentioned earlier) and found that indeed it fixes the assert for the reported scenario: ... $ TERM=dumb ./gdb -q -ex "set width 0" (gdb) ... but I still ran into the assert by typing the command instead of using "-ex": ... $ TERM=dumb ./gdb -q (gdb) set width 0 gdb: display.c:1214: rl_redisplay: Assertion `last_lmargin + (_rl_screenwidth + visible_wrap_offset) <= line_size' failed. Aborted (core dumped) ... Using this additional bit: ... @@ -528,6 +533,8 @@ rl_redisplay () init_line_structures (0); rl_on_new_line (); } + else if (line_size <= _rl_screenwidth) + init_line_structures (_rl_screenwidth + 1); /* Draw the line into the buffer. */ cpos_buffer_position = -1; ... I managed to fix the assert also in this scenario, and managed to run the entire gdb testsuite without triggering the assert. Is that a good code change? Thanks, - Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug-readline] heap-buffer-overflow in update_line 2019-05-23 7:33 ` Tom de Vries @ 2019-05-23 12:38 ` Chet Ramey 2019-05-23 19:28 ` Tom de Vries 0 siblings, 1 reply; 7+ messages in thread From: Chet Ramey @ 2019-05-23 12:38 UTC (permalink / raw) To: Tom de Vries, bug-readline; +Cc: chet.ramey, gdb-patches, Pedro Alves On 5/23/19 3:33 AM, Tom de Vries wrote: > Using this additional bit: > ... > @@ -528,6 +533,8 @@ rl_redisplay () > init_line_structures (0); > rl_on_new_line (); > } > + else if (line_size <= _rl_screenwidth) > + init_line_structures (_rl_screenwidth + 1); > > /* Draw the line into the buffer. */ > cpos_buffer_position = -1; > ... > I managed to fix the assert also in this scenario, and managed to run > the entire gdb testsuite without triggering the assert. > > Is that a good code change? It looks like it will solve that problem, and perhaps more. Thanks for the patch. Chet -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, UTech, CWRU chet@case.edu http://tiswww.cwru.edu/~chet/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug-readline] heap-buffer-overflow in update_line 2019-05-23 12:38 ` Chet Ramey @ 2019-05-23 19:28 ` Tom de Vries 2019-05-24 13:08 ` Chet Ramey 0 siblings, 1 reply; 7+ messages in thread From: Tom de Vries @ 2019-05-23 19:28 UTC (permalink / raw) To: chet.ramey, bug-readline; +Cc: gdb-patches, Pedro Alves [-- Attachment #1: Type: text/plain, Size: 1229 bytes --] On 23-05-19 14:38, Chet Ramey wrote: > On 5/23/19 3:33 AM, Tom de Vries wrote: > >> Using this additional bit: >> ... >> @@ -528,6 +533,8 @@ rl_redisplay () >> init_line_structures (0); >> rl_on_new_line (); >> } >> + else if (line_size <= _rl_screenwidth) >> + init_line_structures (_rl_screenwidth + 1); >> >> /* Draw the line into the buffer. */ >> cpos_buffer_position = -1; >> ... >> I managed to fix the assert also in this scenario, and managed to run >> the entire gdb testsuite without triggering the assert. >> >> Is that a good code change? > > It looks like it will solve that problem, and perhaps more. Thanks for the > patch. I did a further test-run to see if the original problem (PR24514 - heap-buffer-overflow in update_line for utf8-identifiers.exp: https://sourceware.org/bugzilla/show_bug.cgi?id=24514 ) was fixed, which turned out not to be the case. I've analyzed this, and found it to be caused by the init_line_structures part of the patch changing line_size, which does not force a reallloc. I've fixed this by changing minsize instead. Attached patch passes gdb regression testsuite with the assert enabled, and with gdb build with -fsanitize=address. Thanks, - Tom [-- Attachment #2: 0002-tentative.patch --] [-- Type: text/x-patch, Size: 738 bytes --] tentative --- readline/display.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/readline/display.c b/readline/display.c index 712ca79b6d..f34ad05444 100644 --- a/readline/display.c +++ b/readline/display.c @@ -452,6 +452,9 @@ init_line_structures (minsize) { register int n; + if (minsize <= _rl_screenwidth) /* XXX - for gdb */ + minsize = _rl_screenwidth + 1; + if (invisible_line == 0) /* initialize it */ { if (line_size < minsize) @@ -528,6 +531,8 @@ rl_redisplay () init_line_structures (0); rl_on_new_line (); } + else if (line_size <= _rl_screenwidth) + init_line_structures (_rl_screenwidth + 1); /* Draw the line into the buffer. */ cpos_buffer_position = -1; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Bug-readline] heap-buffer-overflow in update_line 2019-05-23 19:28 ` Tom de Vries @ 2019-05-24 13:08 ` Chet Ramey 0 siblings, 0 replies; 7+ messages in thread From: Chet Ramey @ 2019-05-24 13:08 UTC (permalink / raw) To: Tom de Vries, bug-readline; +Cc: chet.ramey, gdb-patches, Pedro Alves On 5/23/19 3:28 PM, Tom de Vries wrote: > I did a further test-run to see if the original problem (PR24514 - > heap-buffer-overflow in update_line for utf8-identifiers.exp: > https://sourceware.org/bugzilla/show_bug.cgi?id=24514 ) was fixed, which > turned out not to be the case. > > I've analyzed this, and found it to be caused by the > init_line_structures part of the patch changing line_size, which does > not force a reallloc. I've fixed this by changing minsize instead. Thanks for the analysis and updated patch. Chet -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, UTech, CWRU chet@case.edu http://tiswww.cwru.edu/~chet/ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-05-24 13:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <52f237e9-83e8-2a97-4766-e60b867ab914@suse.de> [not found] ` <d29fbf1f-d33e-77f6-90ed-0eb3779a027c@case.edu> 2019-05-17 14:59 ` [Bug-readline] heap-buffer-overflow in update_line Tom de Vries 2019-05-17 15:34 ` Chet Ramey 2019-05-20 20:14 ` Chet Ramey 2019-05-23 7:33 ` Tom de Vries 2019-05-23 12:38 ` Chet Ramey 2019-05-23 19:28 ` Tom de Vries 2019-05-24 13:08 ` Chet Ramey
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).