Bug 24980 - Valgrind reports a 'jump on uninit' in readline
Summary: Valgrind reports a 'jump on uninit' in readline
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: cli (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 9.1
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-08 08:42 UTC by philippe.waroquiers
Modified: 2020-06-07 16:40 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description philippe.waroquiers 2019-09-08 08:42:28 UTC
Valgrind reports the following error:
==30026== Conditional jump or move depends on uninitialised value(s)
==30026==    at 0x596A89: rl_redisplay (display.c:710)
==30026==    by 0x582537: readline_internal_setup (readline.c:447)
==30026==    by 0x59C04F: _rl_callback_newline (callback.c:100)
==30026==    by 0x3596A9: gdb_rl_callback_handler_install(char const*) (event-top.c:319)
==30026==    by 0x35A1C6: display_gdb_prompt(char const*) (event-top.c:409)
==30026==    by 0x412824: captured_command_loop() (main.c:328)
==30026==    by 0x41385C: captured_main (main.c:1171)
==30026==    by 0x41385C: gdb_main(captured_main_args*) (main.c:1186)
==30026==    by 0x224B6A: main (gdb.c:32)
==30026==  Uninitialised value was created by a heap allocation
==30026==    at 0x4835753: malloc (vg_replace_malloc.c:307)
==30026==    by 0x25BE07: xmalloc (alloc.c:60)
==30026==    by 0x5931EE: init_line_structures (display.c:614)
==30026==    by 0x597D9E: rl_redisplay (display.c:683)
==30026==    by 0x582537: readline_internal_setup (readline.c:447)
==30026==    by 0x59C04F: _rl_callback_newline (callback.c:100)
==30026==    by 0x3596A9: gdb_rl_callback_handler_install(char const*) (event-top.c:319)
==30026==    by 0x35A1C6: display_gdb_prompt(char const*) (event-top.c:409)
==30026==    by 0x412824: captured_command_loop() (main.c:328)
==30026==    by 0x41385C: captured_main (main.c:1171)
==30026==    by 0x41385C: gdb_main(captured_main_args*) (main.c:1186)
==30026==    by 0x224B6A: main (gdb.c:32)
==30026== 

This seems to have been introduced by 830b67068cebe7db0eb0db3fa19244e03859fae0
This commit has added the following 2 lines in init_line_structures:
  if (minsize <= _rl_screenwidth)	/* XXX - for gdb */
    minsize = _rl_screenwidth + 1;
which means that later on,  (in)visible_line[0..minsize-1] are not initialised:
   for (n = minsize; n < line_size; n++)
     {
       visible_line[n] = 0;
       invisible_line[n] = 1;
     }

Possible fix: in case of very first initialisation (minsize == 0?),
the whole array should be initialised, while subsequent calls
should only initialize the newly allocated part.
Comment 1 Sourceware Commits 2019-09-23 21:36:22 UTC
The master branch has been updated by Andrew Burgess <aburgess@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=32a1adcccf05f98e95a2a451066af810e121bdd9

commit 32a1adcccf05f98e95a2a451066af810e121bdd9
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Wed Sep 18 15:13:25 2019 -0400

    gdb/readline: fix use of an undefined variable
    
    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:
    
    	PR cli/24980
    	* display.c (init_line_structures): Initialise line_state using
    	original minsize value.
Comment 2 Tom Tromey 2020-06-07 16:40:46 UTC
Looks like this was fixed.