From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18380 invoked by alias); 18 Sep 2019 20:24:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18368 invoked by uid 89); 18 Sep 2019 20:24:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.1 spammy=UD:be, philippe X-HELO: mailsec105.isp.belgacom.be Received: from mailsec105.isp.belgacom.be (HELO mailsec105.isp.belgacom.be) (195.238.20.101) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Sep 2019 20:24:36 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1568838276; x=1600374276; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=KxeeGgGb8EgJ7mHxWw9s3wN1lu8KJM7fsaoJYRSgumY=; b=aIbOScizaTIYLmb7+on7BUI6Ij0Kkj4K5oVdA0vjIIL1PNRekCNl+4+z FVPrv4aGhpm8pyhMQxI8luZoKpy36g==; Received: from 255.38-242-81.adsl-dyn.isp.belgacom.be (HELO md) ([81.242.38.255]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 18 Sep 2019 22:24:33 +0200 Message-ID: <5bb4ef3a398af560cc2e0461a3f20e06487b72ba.camel@skynet.be> Subject: Re: [PATCH] gdb/readline: fix use of an undefined variable From: Philippe Waroquiers To: Andrew Burgess , gdb-patches@sourceware.org, bug-readline@gnu.org Cc: Tom de Vries Date: Wed, 18 Sep 2019 20:24:00 -0000 In-Reply-To: <20190918201127.2791-1-andrew.burgess@embecosm.com> References: <20190918201127.2791-1-andrew.burgess@embecosm.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.30.5-1.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00348.txt.bz2 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 > 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: > ;. > Find the GDB manual and other documentation resources online at: > ;. > > 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;