* 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).