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

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

[Andrew Burgess]

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;
-- 
2.14.5

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

[Philippe Waroquiers]

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;

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

[Chet Ramey]

On 9/18/19 4:11 PM, 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:

Thanks for the report and fix.

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/

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

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-09-18 16:11:27 -0400]:

> 
> readline/ChangeLog:
> 
> 	* display.c (init_line_structures): Initialise line_state using
> 	original minsize value.

I've now pushed this to master.

Thanks,
Andrew