Re: [PATCH] gdb/readline: fix use of an undefined variable

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

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;