From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54661 invoked by alias); 19 Feb 2019 22:37:33 -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 54636 invoked by uid 89); 19 Feb 2019 22:37:33 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=sourced, explain, Hx-languages-length:4195, paragraph X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 19 Feb 2019 22:37:31 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AA90C811DB; Tue, 19 Feb 2019 22:37:30 +0000 (UTC) Received: from f29-4.lan (ovpn-117-11.phx2.redhat.com [10.3.117.11]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7B8425D6AA; Tue, 19 Feb 2019 22:37:30 +0000 (UTC) Date: Tue, 19 Feb 2019 22:37:00 -0000 From: Kevin Buettner To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH] Fix error message and use-after-free on errors in nested source files Message-ID: <20190219153729.35348891@f29-4.lan> In-Reply-To: <20190218234640.25664-1-simon.marchi@polymtl.ca> References: <20190218234640.25664-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg00316.txt.bz2 On Mon, 18 Feb 2019 18:46:40 -0500 Simon Marchi wrote: > Errors that happen in nested sourced files (when a sourced file sources > another file) lead to a wrong error message, or use-after-free. > > For example, if I put this in "a.gdb": > > command_that_doesnt_exist > > and this in "b.gdb": > > source a.gdb > > and try to "source b.gdb" in GDB, the result may look like this: > > (gdb) source b.gdb > b.gdb:1: Error in sourced command file: > _that_doesnt_exist:1: Error in sourced command file: > Undefined command: "command_that_doesnt_exist". Try "help". > > Notice the wrong file name where "a.gdb" should be. The exact result > may differ, depending on the feelings of the memory allocator. > > What happens is: > > - The "source a.gdb" command is saved by command_line_append_input_line > in command_line_input's static buffer. > - Since we are sourcing a file, the script_from_file function stores the > script name (a.gdb) in the source_file_name global. However, it doesn't > do a copy, it just saves a pointer to command_line_input's static buffer. > - The "command_that_doesnt_exist" command is saved by > command_line_append_input_line in command_line_input's static buffer. > Depending on what xrealloc does, source_file_name may now point to > freed memory, or at the minimum the data it was pointing to was > overwritten. > - When the error is handled in script_from_file, we dererence > source_file_name to print the name of the file in which the error > occured. > > To fix it, I made source_file_name an std::string, so that keeps a copy of > the file name instead of pointing to a buffer with a too small > lifetime. > > With this patch, the expected filename is printed, and no use-after-free > occurs: > > (gdb) source b.gdb > b.gdb:1: Error in sourced command file: > a.gdb:1: Error in sourced command file: > Undefined command: "command_that_doesnt_exist". Try "help". > > I passed explicit template parameters to make_scoped_restore > (), so that the second parameter is > passed by reference and avoid a copy. > > It was not as obvious as I first thought to change gdb.base/source.exp > to test this, because source commands inside sourced files are > interpreted relative to GDB's current working directory, not the > directory of the currently sourced file. As a workaround, I moved the > snippet that tests errors after the snippet that adds the source > directory to the search path. This way, the "source source-error-1.exp" I think you meant source-error-1.gdb (instead of .exp). > line in source-error.exp manages to find the file. Thanks for this detailed explanation! Might it be possible to (additionally) place some semblance of that last paragraph into the .exp file? (I'm thinking that it might be useful to explain the pitfalls of moving that test from where you have it now to earlier in the file.) The patch looks (mostly) good to me, though I do have a question... > diff --git a/gdb/testsuite/gdb.base/source.exp b/gdb/testsuite/gdb.base/source.exp > index c6ff65783da0..5dd4decf6a5e 100644 > --- a/gdb/testsuite/gdb.base/source.exp > +++ b/gdb/testsuite/gdb.base/source.exp > @@ -23,10 +23,6 @@ standard_testfile structs.c > > gdb_start > > -gdb_test "source ${srcdir}/${subdir}/source-error.gdb" \ > - "source-error.gdb:21: Error in sourced command file:\[\r\n\]*Cannot access memory at address 0x0.*" \ > - "script contains error" > - > gdb_test "source -v ${srcdir}/${subdir}/source-test.gdb" \ > "echo test source options.*" \ > "source -v" > @@ -66,4 +62,9 @@ gdb_test "source for-sure-nonexistant-file" \ > gdb_test "source source-nofile.gdb" \ > "warning: for-sure-nonexistant-file: No such file or directory\.\[\r\n\]*source error not fatal" > > -gdb_exit > + > +gdb_test "source ${srcdir}/${subdir}/source-error.gdb" \ > + [multi_line ".*source-error.gdb:20: Error in sourced command file:" \ > + "source-error-1.gdb:21: Error in sourced command file:" \ > + "Cannot access memory at address 0x0" ] \ > + "script contains error" Did you mean to remove gdb_exit above? If so, why? Kevin