From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7150 invoked by alias); 11 May 2011 03:49:38 -0000 Received: (qmail 7133 invoked by uid 22791); 11 May 2011 03:49:36 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from e24smtp01.br.ibm.com (HELO e24smtp01.br.ibm.com) (32.104.18.85) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 11 May 2011 03:49:21 +0000 Received: from d24relay01.br.ibm.com (d24relay01.br.ibm.com [9.8.31.16]) by e24smtp01.br.ibm.com (8.14.4/8.13.1) with ESMTP id p4B4nVQi024641 for ; Wed, 11 May 2011 00:49:31 -0400 Received: from d24av02.br.ibm.com (d24av02.br.ibm.com [9.8.31.93]) by d24relay01.br.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p4B3kLTr3207422 for ; Wed, 11 May 2011 00:46:21 -0300 Received: from d24av02.br.ibm.com (loopback [127.0.0.1]) by d24av02.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p4B3nHXg022551 for ; Wed, 11 May 2011 00:49:17 -0300 Received: from [9.12.225.42] ([9.12.225.42]) by d24av02.br.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p4B3nD4n022524 for ; Wed, 11 May 2011 00:49:14 -0300 Subject: [RFA] Fix segfault on Python convenience functions which call GDB commands From: Thiago Jung Bauermann To: gdb-patches ml Content-Type: text/plain; charset="UTF-8" Date: Wed, 11 May 2011 03:49:00 -0000 Message-ID: <1305085747.2308.72.camel@hactar> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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 X-SW-Source: 2011-05/txt/msg00267.txt.bz2 Hi, If you have the following convenience function: class Segfault(gdb.Function): """Calls a GDB command to make it segfault.""" def __init__(self): super (Segfault, self).__init__ ("segfault") def invoke(self): return gdb.execute("print 1", to_string=True) Segfault () and call it from GDB, it will live up to its name: hactar% ./gdb -q (gdb) source /tmp/segfault.py (gdb) set var $foo = $segfault() Left operand of assignment is not a modifiable lvalue. (gdb) set var $foo = $segfault() zsh: segmentation fault (core dumped) ./gdb -q When evaluate_subexp_standard evaluates "$foo = $segfault()", it will first evaluate the left hand side, generate a value, then evaluate the right hand side. The problem is that evaluating the RHS involves executing a GDB command. The first thing execute_command does is call prepare_execute_command which in turn calls free_all_values. This nukes the value that evaluate_subexp_standard found for the LHS, leading to a segmentation fault further down the road. The error message about the left operand when first calling the convenience function in the output above is simply because the LHS value is garbage and value->modifiable is 0. We aren't so lucky in the second call and segfault in value_assign. The call to free_all_values in the beginning of execute_command is there since the first version of GDB in the repository. What it does is to free the values created by the command which ran before. This patch fixes the bug by making execute_command (and also mi_cmd_execute, which also calls prepare_execute_command) free all the values created during the execution of the command it just ran instead of leaving them to be cleaned up later. I verified that this patch introduces no memory leaks by running a few testcases under Valgrind's memcheck and massif: break.exp, demangle.exp, funcargs.exp and mi2-var-child.exp. These tests were chosen because they run a fair number of tests against GDB. There are no regressions on ppc-linux, ppc64-linux and x86-linux. -- []'s Thiago Jung Bauermann IBM Linux Technology Center 2011-05-11 Thiago Jung Bauermann gdb/ * mi/mi-main.c (mi_cmd_execute): Use cleanup from prepare_execute_command. top.c (prepare_execute_command): Return cleanup. (execute_command): Use cleanup from prepare_execute_command. * top.h (prepare_execute_command): Change prototype to return cleanup. * defs.h (struct value): Add opaque declaration. (make_cleanup_value_free_to_mark): Add prototype. * utils.c (do_value_free_to_mark): New function. (make_cleanup_value_free_to_mark): Likewise. gdb/testsuite/ * gdb.python/py-function.exp: Test setting a value from a function which executes a command. Index: gdb.git/gdb/mi/mi-main.c =================================================================== --- gdb.git.orig/gdb/mi/mi-main.c 2011-05-11 00:43:29.000000000 -0300 +++ gdb.git/gdb/mi/mi-main.c 2011-05-11 00:43:32.000000000 -0300 @@ -2025,9 +2025,7 @@ mi_cmd_execute (struct mi_parse *parse) { struct cleanup *cleanup; - prepare_execute_command (); - - cleanup = make_cleanup (null_cleanup, NULL); + cleanup = prepare_execute_command (); if (parse->all && parse->thread_group != -1) error (_("Cannot specify --thread-group together with --all")); Index: gdb.git/gdb/top.c =================================================================== --- gdb.git.orig/gdb/top.c 2011-05-11 00:43:29.000000000 -0300 +++ gdb.git/gdb/top.c 2011-05-11 00:43:32.000000000 -0300 @@ -339,10 +339,14 @@ do_chdir_cleanup (void *old_dir) } #endif -void +struct cleanup * prepare_execute_command (void) { - free_all_values (); + struct value *mark; + struct cleanup *cleanup; + + mark = value_mark (); + cleanup = make_cleanup_value_free_to_mark (mark); /* With multiple threads running while the one we're examining is stopped, the dcache can get stale without us being able to detect @@ -350,6 +354,8 @@ prepare_execute_command (void) help things like backtrace. */ if (non_stop) target_dcache_invalidate (); + + return cleanup; } /* Execute the line P as a command, in the current user context. @@ -358,12 +364,13 @@ prepare_execute_command (void) void execute_command (char *p, int from_tty) { + struct cleanup *cleanup; struct cmd_list_element *c; enum language flang; static int warned = 0; char *line; - prepare_execute_command (); + cleanup = prepare_execute_command (); /* Force cleanup of any alloca areas if using C alloca instead of a builtin alloca. */ @@ -462,6 +469,8 @@ execute_command (char *p, int from_tty) warned = 1; } } + + do_cleanups (cleanup); } /* Run execute_command for P and FROM_TTY. Capture its output into the Index: gdb.git/gdb/top.h =================================================================== --- gdb.git.orig/gdb/top.h 2011-05-11 00:43:29.000000000 -0300 +++ gdb.git/gdb/top.h 2011-05-11 00:43:32.000000000 -0300 @@ -48,8 +48,9 @@ extern int quit_cover (void *); extern void execute_command (char *, int); /* Prepare for execution of a command. - Call this before every command, CLI or MI. */ -extern void prepare_execute_command (void); + Call this before every command, CLI or MI. + Returns a cleanup to be run after the command is completed. */ +extern struct cleanup *prepare_execute_command (void); /* This function returns a pointer to the string that is used by gdb for its command prompt. */ Index: gdb.git/gdb/defs.h =================================================================== --- gdb.git.orig/gdb/defs.h 2011-05-11 00:43:29.000000000 -0300 +++ gdb.git/gdb/defs.h 2011-05-11 00:43:32.000000000 -0300 @@ -281,6 +281,7 @@ struct symtab; struct breakpoint; struct frame_info; struct gdbarch; +struct value; /* From main.c. */ @@ -360,6 +361,8 @@ extern struct cleanup *make_cleanup_unpu extern struct cleanup * make_cleanup_restore_ui_file (struct ui_file **variable); +extern struct cleanup * make_cleanup_value_free_to_mark (struct value *); + extern struct cleanup *make_final_cleanup (make_cleanup_ftype *, void *); extern struct cleanup *make_my_cleanup (struct cleanup **, Index: gdb.git/gdb/utils.c =================================================================== --- gdb.git.orig/gdb/utils.c 2011-05-11 00:43:29.000000000 -0300 +++ gdb.git/gdb/utils.c 2011-05-11 00:45:32.000000000 -0300 @@ -431,6 +431,23 @@ make_cleanup_restore_ui_file (struct ui_ return make_cleanup_dtor (do_restore_ui_file, (void *) c, xfree); } +/* Helper for make_cleanup_value_free_to_mark. */ + +static void +do_value_free_to_mark (void *value) +{ + value_free_to_mark ((struct value *) value); +} + +/* Free all values allocated since MARK was obtained by value_mark + (except for those released) when the cleanup is run. */ + +struct cleanup * +make_cleanup_value_free_to_mark (struct value *mark) +{ + return make_my_cleanup (&cleanup_chain, do_value_free_to_mark, mark); +} + struct cleanup * make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function, void *arg, void (*free_arg) (void *)) Index: gdb.git/gdb/testsuite/gdb.python/py-function.exp =================================================================== --- gdb.git.orig/gdb/testsuite/gdb.python/py-function.exp 2011-05-11 00:43:29.000000000 -0300 +++ gdb.git/gdb/testsuite/gdb.python/py-function.exp 2011-05-11 00:43:32.000000000 -0300 @@ -95,3 +95,17 @@ gdb_py_test_multiple "Test Normal Error" gdb_test "print \$normalerror()" "Traceback.*File.*line 5.*in invoke.*RuntimeError.*This is a Normal Error.*" \ "Test a Runtime error. There should be a stack trace." + +gdb_py_test_multiple "input command-calling function" \ + "python" "" \ + "class CallCommand(gdb.Function):" "" \ + " def __init__(self):" "" \ + " gdb.Function.__init__(self, 'call_command')" "" \ + " def invoke(self):" "" \ + " return gdb.execute('print 1', to_string=True)" "" \ + "CallCommand ()" "" \ + "end" "" + +gdb_test_no_output "set var \$foo = \$call_command()" "Setting a value from a function which executes a command." +# There was a bug where GDB would segfault in the second call, so try calling again. +gdb_test_no_output "set var \$foo = \$call_command()" "Setting a value from a function which executes a command, again."