[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-readline] [PATCH] gdb/readline: fix use of an undefined variabl
From: |
Philippe Waroquiers |
Subject: |
Re: [Bug-readline] [PATCH] gdb/readline: fix use of an undefined variable |
Date: |
Wed, 18 Sep 2019 22:24:33 +0200 |
User-agent: |
Evolution 3.30.5-1.1 |
Thanks for the fix.
This problem was reported as
Bug 24980 - Valgrind reports a 'jump on uninit' in readline
https://sourceware.org/bugzilla/show_bug.cgi?id=24980
Philippe
On Wed, 2019-09-18 at 16:11 -0400, Andrew Burgess wrote:
> This commit in binutils-gdb:
>
> commit 830b67068cebe7db0eb0db3fa19244e03859fae0
> Date: Fri Jul 12 09:53:02 2019 +0200
>
> [readline] Fix heap-buffer-overflow in update_line
>
> Which corresponds to this commit in upstream readline:
>
> commit 31547b4ea4a1a904e1b08e2bc4b4ebd5042aedaa
> Date: Mon Aug 5 10:24:27 2019 -0400
>
> commit readline-20190805 snapshot
>
> Introduced a use of an undefined variable, which can be seen using
> valgrind:
>
> $ valgrind --tool=memcheck gdb
> GNU gdb (GDB) 8.3.50.20190918-git
> Copyright (C) 2019 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
> Type "show copying" and "show warranty" for details.
> This GDB was configured as "x86_64-pc-linux-gnu".
> Type "show configuration" for configuration details.
> For bug reporting instructions, please see:
> <http://www.gnu.org/software/gdb/bugs/>;.
> Find the GDB manual and other documentation resources online at:
> <http://www.gnu.org/software/gdb/documentation/>;.
>
> For help, type "help".
> Type "apropos word" to search for commands related to "word".
> ==24924== Conditional jump or move depends on uninitialised value(s)
> ==24924== at 0x9986C3: rl_redisplay (display.c:710)
> ==24924== by 0x9839CE: readline_internal_setup (readline.c:447)
> ==24924== by 0x9A1C2B: _rl_callback_newline (callback.c:100)
> ==24924== by 0x9A1C85: rl_callback_handler_install (callback.c:111)
> ==24924== by 0x6195EB: gdb_rl_callback_handler_install(char const*)
> (event-top.c:319)
> ==24924== by 0x61975E: display_gdb_prompt(char const*) (event-top.c:409)
> ==24924== by 0x4FBFE3: cli_interp_base::pre_command_loop()
> (cli-interp.c:286)
> ==24924== by 0x6E53DA: interp_pre_command_loop(interp*) (interps.c:321)
> ==24924== by 0x731F30: captured_command_loop() (main.c:334)
> ==24924== by 0x733568: captured_main(void*) (main.c:1182)
> ==24924== by 0x7335CE: gdb_main(captured_main_args*) (main.c:1197)
> ==24924== by 0x41325D: main (gdb.c:32)
> ==24924==
> (gdb)
>
> The problem can be traced back to init_line_structures. The very
> first time this function is ever called its MINSIZE parameter is
> always 0 and the global LINE_SIZE is 1024. Prior to the above
> mentioned commits we spot that the line_state variables have not yet
> been initialised, and allocate them some new buffer, then we enter
> this loop:
>
> for (n = minsize; n < line_size; n++)
> {
> visible_line[n] = 0;
> invisible_line[n] = 1;
> }
>
> which would initialise everything from the incoming minimum up to the
> potentially extended upper line size.
>
> The problem is that the above patches added a new condition that would
> bump up the minsize like this:
>
> if (minsize <= _rl_screenwidth) /* XXX - for gdb */
> minsize = _rl_screenwidth + 1;
>
> So, the first time this function is called the incoming MINSIZE is 0,
> the LINE_SIZE global is 1024, and if the _rl_screenwidth is 80, we see
> that MINSIZE will be pushed up to 80. We still notice that the line
> state is uninitialised and allocate some buffers, then we enter the
> initialisation loop:
>
> for (n = minsize; n < line_size; n++)
> {
> visible_line[n] = 0;
> invisible_line[n] = 1;
> }
>
> And initialise from 80 to 1023 i the newly allocated buffers, leaving
> 0 to 79 uninitialised.
>
> To confirm this is an issue, if we then look at rl_redisplay we see
> that a call to init_line_structures is followed first by a call to
> rl_on_new_line, which does initialise visible_line[0], but not
> invisible_line[0]. Later in rl_redisplay we have this logic:
>
> if (visible_line[0] != invisible_line[0])
> rl_display_fixed = 0;
>
> The use of invisible_line[0] here will be undefined.
>
> Considering how this variable was originally initialised before the
> above patches, this patch modifies the initialisation loop in
> init_line_structures, to use the original value of MINSIZE. With this
> change the valgrind warning goes away.
>
> readline/ChangeLog:
>
> * display.c (init_line_structures): Initialise line_state using
> original minsize value.
> ---
> readline/ChangeLog.gdb | 5 +++++
> readline/display.c | 3 ++-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/readline/display.c b/readline/display.c
> index b39f28291be..89193b572bc 100644
> --- a/readline/display.c
> +++ b/readline/display.c
> @@ -602,6 +602,7 @@ static void
> init_line_structures (int minsize)
> {
> register int n;
> + int original_minsize = minsize;
>
> if (minsize <= _rl_screenwidth) /* XXX - for gdb */
> minsize = _rl_screenwidth + 1;
> @@ -622,7 +623,7 @@ init_line_structures (int minsize)
> invisible_line = (char *)xrealloc (invisible_line, line_size);
> }
>
> - for (n = minsize; n < line_size; n++)
> + for (n = original_minsize; n < line_size; n++)
> {
> visible_line[n] = 0;
> invisible_line[n] = 1;