* GDB aborts on missing command args. Which way to fix? @ 2008-09-16 17:50 Paul Pluzhnikov 2008-09-19 21:52 ` Tom Tromey 0 siblings, 1 reply; 6+ messages in thread From: Paul Pluzhnikov @ 2008-09-16 17:50 UTC (permalink / raw) To: gdb-patches Greetings, There are many instances of calls to buildargv() which aren't protected by 'if (args != NULL)', and cause gdb to abort. For example: (gdb) remote del # missing argument to del. Program received signal SIGABRT, Aborted. 0x00002aaaab5cac75 in *__GI_raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 64 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig); (gdb-top) bt #0 0x00002aaaab5cac75 in *__GI_raise (sig=<value optimized out>) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 #1 0x00002aaaab5cc620 in *__GI_abort () at ../sysdeps/generic/abort.c:88 #2 0x000000000044f1a5 in internal_vproblem (problem=0x9ad3c0, file=0x6454db "../../src/gdb/utils.c", line=1002, fmt=<value optimized out>, ap=<value optimized out>) at ../../src/gdb/utils.c:874 #3 0x000000000044c3c9 in internal_verror (file=0x32a <Address 0x32a out of bounds>, line=6, fmt=0xffffffffffffffff <Address 0xffffffffffffffff out of bounds>, ap=0x0) at ../../src/gdb/utils.c:889 #4 0x000000000044c461 in internal_error (file=0x32a <Address 0x32a out of bounds>, line=810, string=0x6 <Address 0x6 out of bounds>) at ../../src/gdb/utils.c:898 #5 0x000000000044cf66 in nomem (size=0) at ../../src/gdb/utils.c:1002 #6 0x0000000000472db1 in remote_delete_command (args=<value optimized out>, from_tty=0) at ../../src/gdb/remote.c:7231 #7 0x000000000044b323 in execute_command (p=0x9d91ca "", from_tty=0) at ../../src/gdb/top.c:457 #8 0x00000000004e9617 in command_handler (command=0x9d91c0 "remote del") at ../../src/gdb/event-top.c:514 #9 0x00000000004ea190 in command_line_handler (rl=<value optimized out>) at ../../src/gdb/event-top.c:739 #10 0x0000000000597303 in rl_callback_read_char () at ../../src/readline/callback.c:205 #11 0x00000000004e9749 in rl_callback_read_char_wrapper (client_data=0x32a) at ../../src/gdb/event-top.c:178 #12 0x00000000004e83a3 in process_event () at ../../src/gdb/event-loop.c:341 #13 0x00000000004e8cb8 in gdb_do_one_event (data=<value optimized out>) at ../../src/gdb/event-loop.c:378 #14 0x00000000004e54db in catch_errors (func=0x4e8b00 <gdb_do_one_event>, func_args=0x0, errstring=0x64d064 "", mask=<value optimized out>) at ../../src/gdb/exceptions.c:516 #15 0x0000000000490276 in tui_command_loop (data=<value optimized out>) at ../../src/gdb/tui/tui-interp.c:153 #16 0x0000000000444139 in captured_command_loop (data=0x32a) at ../../src/gdb/main.c:99 #17 0x00000000004e54db in catch_errors (func=0x444130 <captured_command_loop>, func_args=0x0, errstring=0x64d064 "", mask=<value optimized out>) at ../../src/gdb/exceptions.c:516 #18 0x00000000004448de in captured_main (data=<value optimized out>) at ../../src/gdb/main.c:831 #19 0x00000000004e54db in catch_errors (func=0x444170 <captured_main>, func_args=0x7fffffffe620, errstring=0x64d064 "", mask=<value optimized out>) at ../../src/gdb/exceptions.c:516 #20 0x0000000000444124 in gdb_main (args=0x32a) at ../../src/gdb/main.c:840 #21 0x00000000004440f6 in main (argc=<value optimized out>, argv=0x32a) at ../../src/gdb/gdb.c:33 I can fix this by adding the 'if (args != NULL)' checks everywhere, or by switching to 'buildargv_not_null(args, "appropriate missing argument error")' Which way is preferred? (I prefer the second way). Thanks, -- Paul Pluzhnikov ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: GDB aborts on missing command args. Which way to fix? 2008-09-16 17:50 GDB aborts on missing command args. Which way to fix? Paul Pluzhnikov @ 2008-09-19 21:52 ` Tom Tromey 2008-09-19 22:02 ` Daniel Jacobowitz 0 siblings, 1 reply; 6+ messages in thread From: Tom Tromey @ 2008-09-19 21:52 UTC (permalink / raw) To: Paul Pluzhnikov; +Cc: gdb-patches >>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes: Paul> There are many instances of calls to buildargv() which aren't Paul> protected by 'if (args != NULL)', and cause gdb to abort. [...] Paul> I can fix this by adding the 'if (args != NULL)' checks everywhere, Paul> or by switching to 'buildargv_not_null(args, "appropriate missing Paul> argument error")' Paul> Which way is preferred? (I prefer the second way). That seems reasonable to me. I think you might as well make it call nomem if the result is NULL, too. Tom ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: GDB aborts on missing command args. Which way to fix? 2008-09-19 21:52 ` Tom Tromey @ 2008-09-19 22:02 ` Daniel Jacobowitz 2008-09-26 23:22 ` [patch] " Paul Pluzhnikov 0 siblings, 1 reply; 6+ messages in thread From: Daniel Jacobowitz @ 2008-09-19 22:02 UTC (permalink / raw) To: Tom Tromey; +Cc: Paul Pluzhnikov, gdb-patches On Fri, Sep 19, 2008 at 03:50:03PM -0600, Tom Tromey wrote: > >>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes: > > Paul> There are many instances of calls to buildargv() which aren't > Paul> protected by 'if (args != NULL)', and cause gdb to abort. > [...] > Paul> I can fix this by adding the 'if (args != NULL)' checks everywhere, > Paul> or by switching to 'buildargv_not_null(args, "appropriate missing > Paul> argument error")' > > Paul> Which way is preferred? (I prefer the second way). > > That seems reasonable to me. > > I think you might as well make it call nomem if the result is NULL, > too. I don't like passing error messages to functions that might fail; I find it very confusing. If you want to give usage errors, why not combine it with the existing checks for wrong arguments? Tom's got a good point about nomem, though. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 6+ messages in thread
* [patch] Re: GDB aborts on missing command args. Which way to fix? 2008-09-19 22:02 ` Daniel Jacobowitz @ 2008-09-26 23:22 ` Paul Pluzhnikov 2008-10-03 7:04 ` Joel Brobecker 0 siblings, 1 reply; 6+ messages in thread From: Paul Pluzhnikov @ 2008-09-26 23:22 UTC (permalink / raw) To: Tom Tromey, gdb-patches On Fri, Sep 19, 2008 at 3:01 PM, Daniel Jacobowitz <drow@false.org> wrote: > I don't like passing error messages to functions that might fail; I ... > Tom's got a good point about nomem, though. Ok. Tested on Linux/x86_64, no regressions. -- Paul Pluzhnikov 2008-09-26 Paul Pluzhnikov <ppluzhnikov@google.com> * utils.c, defs.h (gdb_buildargv): New fn. Wrap buildargv and check for out-of-memory condition. * exec.c (exec_file_command): Call it. * infrun.c (handle_command, xdb_handle_command): Likewise. * interps.c (interpreter_exec_cmd): Likewise. * linux-nat.c (linux_nat_info_proc_cmd): Likewise. * procfs.c (info_proc_cmd): Likewise. * remote-mips.c (common_open): Likewise. * remote-sim.c (gdbsim_kill, gdbsim_create_inferior, gdbsim_open): Likewise. * remote.c (extended_remote_run, remote_put_command, remote_get_command, remote_delete_command): Likewise. * ser-mingw.c (pipe_windows_open): Likesise. * source.c (add_path, show_substitute_path_command, unset_substitute_path_command, set_substitute_path_command): Likewise. * stack.c (backtrace_command): Likewise. * symfile.c (symbol_file_command, generic_load, add_symbol_file_command): Likesise. * symmisc.c (maintenance_print_symbols, maintenance_print_psymbols, maintenance_print_msymbols): Likewise. Index: defs.h =================================================================== RCS file: /cvs/src/src/gdb/defs.h,v retrieving revision 1.237 diff -u -d -p -u -r1.237 defs.h --- defs.h 14 Sep 2008 06:37:18 -0000 1.237 +++ defs.h 26 Sep 2008 22:45:22 -0000 @@ -405,6 +405,8 @@ ULONGEST strtoulst (const char *num, con char *ldirname (const char *filename); +char **gdb_buildargv (const char *); + /* From demangle.c */ extern void set_demangling_style (char *); Index: exec.c =================================================================== RCS file: /cvs/src/src/gdb/exec.c,v retrieving revision 1.77 diff -u -d -p -u -r1.77 exec.c --- exec.c 5 Sep 2008 11:37:17 -0000 1.77 +++ exec.c 26 Sep 2008 22:45:22 -0000 @@ -302,10 +302,7 @@ exec_file_command (char *args, int from_ /* Scan through the args and pick up the first non option arg as the filename. */ - argv = buildargv (args); - if (argv == NULL) - nomem (0); - + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); for (; (*argv != NULL) && (**argv == '-'); argv++) Index: infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.322 diff -u -d -p -u -r1.322 infrun.c --- infrun.c 22 Sep 2008 15:26:53 -0000 1.322 +++ infrun.c 26 Sep 2008 22:45:22 -0000 @@ -4070,11 +4070,7 @@ handle_command (char *args, int from_tty /* Break the command line up into args. */ - argv = buildargv (args); - if (argv == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); old_chain = make_cleanup_freeargv (argv); /* Walk through the args, looking for signal oursigs, signal names, and @@ -4231,13 +4227,12 @@ xdb_handle_command (char *args, int from char **argv; struct cleanup *old_chain; + if (args == NULL) + error_no_arg (_("xdb command")); + /* Break the command line up into args. */ - argv = buildargv (args); - if (argv == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); old_chain = make_cleanup_freeargv (argv); if (argv[1] != (char *) NULL) { Index: interps.c =================================================================== RCS file: /cvs/src/src/gdb/interps.c,v retrieving revision 1.28 diff -u -d -p -u -r1.28 interps.c --- interps.c 28 Jul 2008 17:53:52 -0000 1.28 +++ interps.c 26 Sep 2008 22:45:22 -0000 @@ -371,11 +371,11 @@ interpreter_exec_cmd (char *args, int fr unsigned int i; int old_quiet, use_quiet; - prules = buildargv (args); + prules = gdb_buildargv (args); if (prules == NULL) - { - error (_("unable to parse arguments")); - } + error (_("unable to parse arguments")); + else + make_cleanup_freeargv (prules); nrules = 0; if (prules != NULL) Index: linux-nat.c =================================================================== RCS file: /cvs/src/src/gdb/linux-nat.c,v retrieving revision 1.106 diff -u -d -p -u -r1.106 linux-nat.c --- linux-nat.c 25 Sep 2008 14:13:44 -0000 1.106 +++ linux-nat.c 26 Sep 2008 22:45:22 -0000 @@ -3591,10 +3591,8 @@ linux_nat_info_proc_cmd (char *args, int if (args) { /* Break up 'args' into an argv array. */ - if ((argv = buildargv (args)) == NULL) - nomem (0); - else - make_cleanup_freeargv (argv); + argv = gdb_buildargv (args); + make_cleanup_freeargv (argv); } while (argv != NULL && *argv != NULL) { Index: procfs.c =================================================================== RCS file: /cvs/src/src/gdb/procfs.c,v retrieving revision 1.94 diff -u -d -p -u -r1.94 procfs.c --- procfs.c 22 Sep 2008 15:21:30 -0000 1.94 +++ procfs.c 26 Sep 2008 22:45:22 -0000 @@ -5853,10 +5853,8 @@ info_proc_cmd (char *args, int from_tty) old_chain = make_cleanup (null_cleanup, 0); if (args) { - if ((argv = buildargv (args)) == NULL) - nomem (0); - else - make_cleanup_freeargv (argv); + argv = gdb_buildargv (args); + make_cleanup_freeargv (argv); } while (argv != NULL && *argv != NULL) { Index: remote-mips.c =================================================================== RCS file: /cvs/src/src/gdb/remote-mips.c,v retrieving revision 1.90 diff -u -d -p -u -r1.90 remote-mips.c --- remote-mips.c 4 Sep 2008 22:49:30 -0000 1.90 +++ remote-mips.c 26 Sep 2008 22:45:23 -0000 @@ -1490,8 +1490,7 @@ device is attached to the target board ( /* Parse the serial port name, the optional TFTP name, and the optional local TFTP name. */ - if ((argv = buildargv (name)) == NULL) - nomem (0); + argv = gdb_buildargv (name); make_cleanup_freeargv (argv); serial_port_name = xstrdup (argv[0]); Index: remote-sim.c =================================================================== RCS file: /cvs/src/src/gdb/remote-sim.c,v retrieving revision 1.76 diff -u -d -p -u -r1.76 remote-sim.c --- remote-sim.c 24 Sep 2008 16:37:24 -0000 1.76 +++ remote-sim.c 26 Sep 2008 22:45:23 -0000 @@ -406,11 +406,11 @@ gdbsim_kill (void) static void gdbsim_load (char *args, int fromtty) { - char **argv = buildargv (args); + char **argv = gdb_buildargv (args); char *prog; - if (argv == NULL) - nomem (0); + if (args == NULL) + error_no_arg (_("program to load")); make_cleanup_freeargv (argv); @@ -472,7 +472,7 @@ gdbsim_create_inferior (char *exec_file, strcat (arg_buf, exec_file); strcat (arg_buf, " "); strcat (arg_buf, args); - argv = buildargv (arg_buf); + argv = gdb_buildargv (arg_buf); make_cleanup_freeargv (argv); } else @@ -546,9 +546,7 @@ gdbsim_open (char *args, int from_tty) strcat (arg_buf, " "); /* 1 */ strcat (arg_buf, args); } - argv = buildargv (arg_buf); - if (argv == NULL) - error (_("Insufficient memory available to allocate simulator arg list.")); + argv = gdb_buildargv (arg_buf); make_cleanup_freeargv (argv); init_callbacks (); Index: remote.c =================================================================== RCS file: /cvs/src/src/gdb/remote.c,v retrieving revision 1.310 diff -u -d -p -u -r1.310 remote.c --- remote.c 22 Sep 2008 15:21:30 -0000 1.310 +++ remote.c 26 Sep 2008 22:45:23 -0000 @@ -5546,13 +5546,16 @@ extended_remote_run (char *args) error (_("Remote file name too long for run packet")); len += 2 * bin2hex ((gdb_byte *) remote_exec_file, rs->buf + len, 0); + if (args == NULL) + error_no_arg (_("remote arguments")); + if (*args) { struct cleanup *back_to; int i; char **argv; - argv = buildargv (args); + argv = gdb_buildargv (args); back_to = make_cleanup ((void (*) (void *)) freeargv, argv); for (i = 0; argv[i] != NULL; i++) { @@ -7491,9 +7494,10 @@ remote_put_command (char *args, int from struct cleanup *back_to; char **argv; - argv = buildargv (args); - if (argv == NULL) - nomem (0); + if (args == NULL) + error_no_arg (_("file to put")); + + argv = gdb_buildargv (args); back_to = make_cleanup_freeargv (argv); if (argv[0] == NULL || argv[1] == NULL || argv[2] != NULL) error (_("Invalid parameters to remote put")); @@ -7509,9 +7513,10 @@ remote_get_command (char *args, int from struct cleanup *back_to; char **argv; - argv = buildargv (args); - if (argv == NULL) - nomem (0); + if (args == NULL) + error_no_arg (_("file to get")); + + argv = gdb_buildargv (args); back_to = make_cleanup_freeargv (argv); if (argv[0] == NULL || argv[1] == NULL || argv[2] != NULL) error (_("Invalid parameters to remote get")); @@ -7527,9 +7532,10 @@ remote_delete_command (char *args, int f struct cleanup *back_to; char **argv; - argv = buildargv (args); - if (argv == NULL) - nomem (0); + if (args == NULL) + error_no_arg (_("file to delete")); + + argv = gdb_buildargv (args); back_to = make_cleanup_freeargv (argv); if (argv[0] == NULL || argv[1] != NULL) error (_("Invalid parameters to remote delete")); Index: ser-mingw.c =================================================================== RCS file: /cvs/src/src/gdb/ser-mingw.c,v retrieving revision 1.14 diff -u -d -p -u -r1.14 ser-mingw.c --- ser-mingw.c 3 Sep 2008 23:54:19 -0000 1.14 +++ ser-mingw.c 26 Sep 2008 22:45:23 -0000 @@ -818,12 +818,15 @@ pipe_windows_open (struct serial *scb, c struct pipe_state *ps; FILE *pex_stderr; - char **argv = buildargv (name); + if (name == NULL) + error_no_arg (_("child command")); + + char **argv = gdb_buildargv (name); struct cleanup *back_to = make_cleanup_freeargv (argv); + if (! argv[0] || argv[0][0] == '\0') error ("missing child command"); - ps = make_pipe_state (); make_cleanup (cleanup_pipe_state, ps); Index: source.c =================================================================== RCS file: /cvs/src/src/gdb/source.c,v retrieving revision 1.91 diff -u -d -p -u -r1.91 source.c --- source.c 11 Sep 2008 14:21:49 -0000 1.91 +++ source.c 26 Sep 2008 22:45:23 -0000 @@ -428,12 +428,9 @@ add_path (char *dirname, char **which_pa /* This will properly parse the space and tab separators and any quotes that may exist. DIRNAME_SEPARATOR will be dealt with later. */ - argv = buildargv (dirname); + argv = gdb_buildargv (dirname); make_cleanup_freeargv (argv); - if (argv == NULL) - nomem (0); - arg = argv[0]; } else @@ -1813,7 +1810,7 @@ show_substitute_path_command (char *args char **argv; char *from = NULL; - argv = buildargv (args); + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); /* We expect zero or one argument. */ @@ -1846,7 +1843,7 @@ static void unset_substitute_path_command (char *args, int from_tty) { struct substitute_path_rule *rule = substitute_path_rules; - char **argv = buildargv (args); + char **argv = gdb_buildargv (args); char *from = NULL; int rule_found = 0; @@ -1899,7 +1896,7 @@ set_substitute_path_command (char *args, char **argv; struct substitute_path_rule *rule; - argv = buildargv (args); + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); if (argv == NULL || argv[0] == NULL || argv [1] == NULL) Index: stack.c =================================================================== RCS file: /cvs/src/src/gdb/stack.c,v retrieving revision 1.179 diff -u -d -p -u -r1.179 stack.c --- stack.c 25 Sep 2008 16:04:11 -0000 1.179 +++ stack.c 26 Sep 2008 22:45:23 -0000 @@ -1282,7 +1282,7 @@ backtrace_command (char *arg, int from_t char **argv; int i; - argv = buildargv (arg); + argv = gdb_buildargv (arg); old_chain = make_cleanup_freeargv (argv); argc = 0; for (i = 0; argv[i]; i++) Index: symfile.c =================================================================== RCS file: /cvs/src/src/gdb/symfile.c,v retrieving revision 1.216 diff -u -d -p -u -r1.216 symfile.c --- symfile.c 22 Sep 2008 18:18:07 -0000 1.216 +++ symfile.c 26 Sep 2008 22:45:23 -0000 @@ -1488,14 +1488,11 @@ symbol_file_command (char *args, int fro } else { - char **argv = buildargv (args); + char **argv = gdb_buildargv (args); int flags = OBJF_USERLOADED; struct cleanup *cleanups; char *name = NULL; - if (argv == NULL) - nomem (0); - cleanups = make_cleanup_freeargv (argv); while (*argv != NULL) { @@ -1929,11 +1926,7 @@ generic_load (char *args, int from_tty) make_cleanup (clear_memory_write_data, &cbdata.requests); - argv = buildargv (args); - - if (argv == NULL) - nomem(0); - + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); filename = tilde_expand (argv[0]); @@ -2122,12 +2115,9 @@ add_symbol_file_command (char *args, int if (args == NULL) error (_("add-symbol-file takes a file name and an address")); - argv = buildargv (args); + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); - if (argv == NULL) - nomem (0); - for (arg = argv[0], argcnt = 0; arg != NULL; arg = argv[++argcnt]) { /* Process the argument. */ Index: symmisc.c =================================================================== RCS file: /cvs/src/src/gdb/symmisc.c,v retrieving revision 1.56 diff -u -d -p -u -r1.56 symmisc.c --- symmisc.c 5 Sep 2008 11:37:17 -0000 1.56 +++ symmisc.c 26 Sep 2008 22:45:23 -0000 @@ -526,10 +526,7 @@ maintenance_print_symbols (char *args, i error (_("\ Arguments missing: an output file name and an optional symbol file name")); } - else if ((argv = buildargv (args)) == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); cleanups = make_cleanup_freeargv (argv); if (argv[0] != NULL) @@ -739,10 +736,7 @@ maintenance_print_psymbols (char *args, { error (_("print-psymbols takes an output file name and optional symbol file name")); } - else if ((argv = buildargv (args)) == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); cleanups = make_cleanup_freeargv (argv); if (argv[0] != NULL) @@ -878,10 +872,7 @@ maintenance_print_msymbols (char *args, { error (_("print-msymbols takes an output file name and optional symbol file name")); } - else if ((argv = buildargv (args)) == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); cleanups = make_cleanup_freeargv (argv); if (argv[0] != NULL) Index: utils.c =================================================================== RCS file: /cvs/src/src/gdb/utils.c,v retrieving revision 1.195 diff -u -d -p -u -r1.195 utils.c --- utils.c 8 Sep 2008 21:57:42 -0000 1.195 +++ utils.c 26 Sep 2008 22:45:23 -0000 @@ -3349,3 +3349,12 @@ ldirname (const char *filename) dirname[base - filename] = '\0'; return dirname; } + +char ** +gdb_buildargv (const char *s) +{ + char **argv = buildargv (s); + if (s != NULL && argv == NULL) + nomem (0); + return argv; +} ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Re: GDB aborts on missing command args. Which way to fix? 2008-09-26 23:22 ` [patch] " Paul Pluzhnikov @ 2008-10-03 7:04 ` Joel Brobecker 2008-10-03 16:39 ` Paul Pluzhnikov 0 siblings, 1 reply; 6+ messages in thread From: Joel Brobecker @ 2008-10-03 7:04 UTC (permalink / raw) To: Paul Pluzhnikov; +Cc: Tom Tromey, gdb-patches > * remote-sim.c (gdbsim_kill, gdbsim_create_inferior, > gdbsim_open): Likewise. Minor detail. You need to close the parens on the first line, and then reopen it on the next line. This is what the GNU Coding Standards require. So: * remote-sim.c (gdbsim_kill, gdbsim_create_inferior) (gdbsim_open): Likewise. You have other cases of the same issues in the changelog, so please be sure to fix these as well. > @@ -371,11 +371,11 @@ interpreter_exec_cmd (char *args, int fr > unsigned int i; > int old_quiet, use_quiet; > > - prules = buildargv (args); > + prules = gdb_buildargv (args); > if (prules == NULL) > - { > - error (_("unable to parse arguments")); > - } > + error (_("unable to parse arguments")); > + else > + make_cleanup_freeargv (prules); I think it would be nice to be consistent in the way we are handling the argument. In this case, it's a bit obscure at first sight what it would mean for prules to be NULL. You have to know the semantics of gdb_buildargv and determine from there that the only way it can be null is if args is NULL as well. How about we change this to: if (args == NULL) error_no_arg (_("interpreter-exec command")); prules = gdb_buildargv (args) make_cleanup_freeargv (prules) ? > - char **argv = buildargv (args); > + char **argv = gdb_buildargv (args); > char *prog; > > - if (argv == NULL) > - nomem (0); > + if (args == NULL) > + error_no_arg (_("program to load")); I guess I'm being a little anal about this, but it's strange to be using args before you check whether it's null or not. I know gdb_buildargv would return NULL is args is NULL, but can you move the call after the check for args. > @@ -5546,13 +5546,16 @@ extended_remote_run (char *args) > error (_("Remote file name too long for run packet")); > len += 2 * bin2hex ((gdb_byte *) remote_exec_file, rs->buf + len, 0); > > + if (args == NULL) > + error_no_arg (_("remote arguments")); This should never happen. There are a few indirections involved, but essentially thiis function is called from "run_command" in infcmd, and ARGS is pretty much guaranteed to be non NULL. Two options: Add a "gdb_assert (args != NULL);" or make the function handle the case where args is NULL the same as if args == "". For that, changing the following if should work: > if (*args) if (args && *args). > @@ -1929,11 +1926,7 @@ generic_load (char *args, int from_tty) > > make_cleanup (clear_memory_write_data, &cbdata.requests); > > - argv = buildargv (args); > - > - if (argv == NULL) > - nomem(0); > - > + argv = gdb_buildargv (args); > make_cleanup_freeargv (argv); I think this one is missing the check for args first. The rest of this function is accessing argv freely, so it's definitely expecting argv to be non-null, which means args must be non-null. > + > +char ** > +gdb_buildargv (const char *s) > +{ > + char **argv = buildargv (s); > + if (s != NULL && argv == NULL) > + nomem (0); > + return argv; > +} Can you add a comment just before the function that explains what the function does, exactly. I think it's fine to describe the function compared to what libiberty's buildargv does. Something like this: /* Call libiberty's buildargv, and return the result, except that it calls nomem if buildargv ran out of memory. Therefore, the returned value is guarantied to be non-NULL unless S is null. */ Your patch is pre-approved with the corrections mentioned above. Nice cleanup! -- Joel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] Re: GDB aborts on missing command args. Which way to fix? 2008-10-03 7:04 ` Joel Brobecker @ 2008-10-03 16:39 ` Paul Pluzhnikov 0 siblings, 0 replies; 6+ messages in thread From: Paul Pluzhnikov @ 2008-10-03 16:39 UTC (permalink / raw) To: Joel Brobecker; +Cc: Tom Tromey, gdb-patches [-- Attachment #1: Type: text/plain, Size: 216 bytes --] On Fri, Oct 3, 2008 at 12:03 AM, Joel Brobecker <brobecker@adacore.com> wrote: > Your patch is pre-approved with the corrections mentioned above. Thanks for the review. Checked in as attached. -- Paul Pluzhnikov [-- Attachment #2: gdb-buildargv-20081003.txt --] [-- Type: text/plain, Size: 15473 bytes --] 2008-10-03 Paul Pluzhnikov <ppluzhnikov@google.com> * utils.c, defs.h (gdb_buildargv): New fn. Wrap buildargv and check for out-of-memory condition. * exec.c (exec_file_command): Call it. * infrun.c (handle_command, xdb_handle_command): Likewise. * interps.c (interpreter_exec_cmd): Likewise. * linux-nat.c (linux_nat_info_proc_cmd): Likewise. * procfs.c (info_proc_cmd): Likewise. * remote-mips.c (common_open): Likewise. * remote-sim.c (gdbsim_kill, gdbsim_create_inferior) (gdbsim_open): Likewise. * remote.c (extended_remote_run, remote_put_command) (remote_get_command, remote_delete_command): Likewise. * ser-mingw.c (pipe_windows_open): Likesise. * source.c (add_path, show_substitute_path_command) (unset_substitute_path_command, set_substitute_path_command): Likewise. * stack.c (backtrace_command): Likewise. * symfile.c (symbol_file_command, generic_load) (add_symbol_file_command): Likesise. * symmisc.c (maintenance_print_symbols, maintenance_print_psymbols) (maintenance_print_msymbols): Likewise. Index: defs.h =================================================================== RCS file: /cvs/src/src/gdb/defs.h,v retrieving revision 1.237 diff -u -p -u -r1.237 defs.h --- defs.h 14 Sep 2008 06:37:18 -0000 1.237 +++ defs.h 3 Oct 2008 16:28:34 -0000 @@ -405,6 +405,8 @@ ULONGEST strtoulst (const char *num, con char *ldirname (const char *filename); +char **gdb_buildargv (const char *); + /* From demangle.c */ extern void set_demangling_style (char *); Index: exec.c =================================================================== RCS file: /cvs/src/src/gdb/exec.c,v retrieving revision 1.77 diff -u -p -u -r1.77 exec.c --- exec.c 5 Sep 2008 11:37:17 -0000 1.77 +++ exec.c 3 Oct 2008 16:28:34 -0000 @@ -302,10 +302,7 @@ exec_file_command (char *args, int from_ /* Scan through the args and pick up the first non option arg as the filename. */ - argv = buildargv (args); - if (argv == NULL) - nomem (0); - + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); for (; (*argv != NULL) && (**argv == '-'); argv++) Index: infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.323 diff -u -p -u -r1.323 infrun.c --- infrun.c 2 Oct 2008 15:48:06 -0000 1.323 +++ infrun.c 3 Oct 2008 16:28:35 -0000 @@ -4070,11 +4070,7 @@ handle_command (char *args, int from_tty /* Break the command line up into args. */ - argv = buildargv (args); - if (argv == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); old_chain = make_cleanup_freeargv (argv); /* Walk through the args, looking for signal oursigs, signal names, and @@ -4231,13 +4227,12 @@ xdb_handle_command (char *args, int from char **argv; struct cleanup *old_chain; + if (args == NULL) + error_no_arg (_("xdb command")); + /* Break the command line up into args. */ - argv = buildargv (args); - if (argv == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); old_chain = make_cleanup_freeargv (argv); if (argv[1] != (char *) NULL) { Index: interps.c =================================================================== RCS file: /cvs/src/src/gdb/interps.c,v retrieving revision 1.28 diff -u -p -u -r1.28 interps.c --- interps.c 28 Jul 2008 17:53:52 -0000 1.28 +++ interps.c 3 Oct 2008 16:28:35 -0000 @@ -371,20 +371,15 @@ interpreter_exec_cmd (char *args, int fr unsigned int i; int old_quiet, use_quiet; - prules = buildargv (args); - if (prules == NULL) - { - error (_("unable to parse arguments")); - } + if (args == NULL) + error_no_arg (_("interpreter-exec command")); + + prules = gdb_buildargv (args); + make_cleanup_freeargv (prules); nrules = 0; - if (prules != NULL) - { - for (trule = prules; *trule != NULL; trule++) - { - nrules++; - } - } + for (trule = prules; *trule != NULL; trule++) + nrules++; if (nrules < 2) error (_("usage: interpreter-exec <interpreter> [ <command> ... ]")); Index: linux-nat.c =================================================================== RCS file: /cvs/src/src/gdb/linux-nat.c,v retrieving revision 1.106 diff -u -p -u -r1.106 linux-nat.c --- linux-nat.c 25 Sep 2008 14:13:44 -0000 1.106 +++ linux-nat.c 3 Oct 2008 16:28:35 -0000 @@ -3591,10 +3591,8 @@ linux_nat_info_proc_cmd (char *args, int if (args) { /* Break up 'args' into an argv array. */ - if ((argv = buildargv (args)) == NULL) - nomem (0); - else - make_cleanup_freeargv (argv); + argv = gdb_buildargv (args); + make_cleanup_freeargv (argv); } while (argv != NULL && *argv != NULL) { Index: procfs.c =================================================================== RCS file: /cvs/src/src/gdb/procfs.c,v retrieving revision 1.94 diff -u -p -u -r1.94 procfs.c --- procfs.c 22 Sep 2008 15:21:30 -0000 1.94 +++ procfs.c 3 Oct 2008 16:28:35 -0000 @@ -5853,10 +5853,8 @@ info_proc_cmd (char *args, int from_tty) old_chain = make_cleanup (null_cleanup, 0); if (args) { - if ((argv = buildargv (args)) == NULL) - nomem (0); - else - make_cleanup_freeargv (argv); + argv = gdb_buildargv (args); + make_cleanup_freeargv (argv); } while (argv != NULL && *argv != NULL) { Index: remote-mips.c =================================================================== RCS file: /cvs/src/src/gdb/remote-mips.c,v retrieving revision 1.90 diff -u -p -u -r1.90 remote-mips.c --- remote-mips.c 4 Sep 2008 22:49:30 -0000 1.90 +++ remote-mips.c 3 Oct 2008 16:28:35 -0000 @@ -1490,8 +1490,7 @@ device is attached to the target board ( /* Parse the serial port name, the optional TFTP name, and the optional local TFTP name. */ - if ((argv = buildargv (name)) == NULL) - nomem (0); + argv = gdb_buildargv (name); make_cleanup_freeargv (argv); serial_port_name = xstrdup (argv[0]); Index: remote-sim.c =================================================================== RCS file: /cvs/src/src/gdb/remote-sim.c,v retrieving revision 1.76 diff -u -p -u -r1.76 remote-sim.c --- remote-sim.c 24 Sep 2008 16:37:24 -0000 1.76 +++ remote-sim.c 3 Oct 2008 16:28:35 -0000 @@ -406,12 +406,13 @@ gdbsim_kill (void) static void gdbsim_load (char *args, int fromtty) { - char **argv = buildargv (args); + char **argv; char *prog; - if (argv == NULL) - nomem (0); + if (args == NULL) + error_no_arg (_("program to load")); + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); prog = tilde_expand (argv[0]); @@ -472,7 +473,7 @@ gdbsim_create_inferior (char *exec_file, strcat (arg_buf, exec_file); strcat (arg_buf, " "); strcat (arg_buf, args); - argv = buildargv (arg_buf); + argv = gdb_buildargv (arg_buf); make_cleanup_freeargv (argv); } else @@ -546,9 +547,7 @@ gdbsim_open (char *args, int from_tty) strcat (arg_buf, " "); /* 1 */ strcat (arg_buf, args); } - argv = buildargv (arg_buf); - if (argv == NULL) - error (_("Insufficient memory available to allocate simulator arg list.")); + argv = gdb_buildargv (arg_buf); make_cleanup_freeargv (argv); init_callbacks (); Index: remote.c =================================================================== RCS file: /cvs/src/src/gdb/remote.c,v retrieving revision 1.310 diff -u -p -u -r1.310 remote.c --- remote.c 22 Sep 2008 15:21:30 -0000 1.310 +++ remote.c 3 Oct 2008 16:28:35 -0000 @@ -5546,13 +5546,14 @@ extended_remote_run (char *args) error (_("Remote file name too long for run packet")); len += 2 * bin2hex ((gdb_byte *) remote_exec_file, rs->buf + len, 0); + gdb_assert (args != NULL); if (*args) { struct cleanup *back_to; int i; char **argv; - argv = buildargv (args); + argv = gdb_buildargv (args); back_to = make_cleanup ((void (*) (void *)) freeargv, argv); for (i = 0; argv[i] != NULL; i++) { @@ -7491,9 +7492,10 @@ remote_put_command (char *args, int from struct cleanup *back_to; char **argv; - argv = buildargv (args); - if (argv == NULL) - nomem (0); + if (args == NULL) + error_no_arg (_("file to put")); + + argv = gdb_buildargv (args); back_to = make_cleanup_freeargv (argv); if (argv[0] == NULL || argv[1] == NULL || argv[2] != NULL) error (_("Invalid parameters to remote put")); @@ -7509,9 +7511,10 @@ remote_get_command (char *args, int from struct cleanup *back_to; char **argv; - argv = buildargv (args); - if (argv == NULL) - nomem (0); + if (args == NULL) + error_no_arg (_("file to get")); + + argv = gdb_buildargv (args); back_to = make_cleanup_freeargv (argv); if (argv[0] == NULL || argv[1] == NULL || argv[2] != NULL) error (_("Invalid parameters to remote get")); @@ -7527,9 +7530,10 @@ remote_delete_command (char *args, int f struct cleanup *back_to; char **argv; - argv = buildargv (args); - if (argv == NULL) - nomem (0); + if (args == NULL) + error_no_arg (_("file to delete")); + + argv = gdb_buildargv (args); back_to = make_cleanup_freeargv (argv); if (argv[0] == NULL || argv[1] != NULL) error (_("Invalid parameters to remote delete")); Index: ser-mingw.c =================================================================== RCS file: /cvs/src/src/gdb/ser-mingw.c,v retrieving revision 1.14 diff -u -p -u -r1.14 ser-mingw.c --- ser-mingw.c 3 Sep 2008 23:54:19 -0000 1.14 +++ ser-mingw.c 3 Oct 2008 16:28:35 -0000 @@ -818,12 +818,15 @@ pipe_windows_open (struct serial *scb, c struct pipe_state *ps; FILE *pex_stderr; - char **argv = buildargv (name); + if (name == NULL) + error_no_arg (_("child command")); + + char **argv = gdb_buildargv (name); struct cleanup *back_to = make_cleanup_freeargv (argv); + if (! argv[0] || argv[0][0] == '\0') error ("missing child command"); - ps = make_pipe_state (); make_cleanup (cleanup_pipe_state, ps); Index: source.c =================================================================== RCS file: /cvs/src/src/gdb/source.c,v retrieving revision 1.91 diff -u -p -u -r1.91 source.c --- source.c 11 Sep 2008 14:21:49 -0000 1.91 +++ source.c 3 Oct 2008 16:28:35 -0000 @@ -428,12 +428,9 @@ add_path (char *dirname, char **which_pa /* This will properly parse the space and tab separators and any quotes that may exist. DIRNAME_SEPARATOR will be dealt with later. */ - argv = buildargv (dirname); + argv = gdb_buildargv (dirname); make_cleanup_freeargv (argv); - if (argv == NULL) - nomem (0); - arg = argv[0]; } else @@ -1813,7 +1810,7 @@ show_substitute_path_command (char *args char **argv; char *from = NULL; - argv = buildargv (args); + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); /* We expect zero or one argument. */ @@ -1846,7 +1843,7 @@ static void unset_substitute_path_command (char *args, int from_tty) { struct substitute_path_rule *rule = substitute_path_rules; - char **argv = buildargv (args); + char **argv = gdb_buildargv (args); char *from = NULL; int rule_found = 0; @@ -1899,7 +1896,7 @@ set_substitute_path_command (char *args, char **argv; struct substitute_path_rule *rule; - argv = buildargv (args); + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); if (argv == NULL || argv[0] == NULL || argv [1] == NULL) Index: stack.c =================================================================== RCS file: /cvs/src/src/gdb/stack.c,v retrieving revision 1.179 diff -u -p -u -r1.179 stack.c --- stack.c 25 Sep 2008 16:04:11 -0000 1.179 +++ stack.c 3 Oct 2008 16:28:35 -0000 @@ -1282,7 +1282,7 @@ backtrace_command (char *arg, int from_t char **argv; int i; - argv = buildargv (arg); + argv = gdb_buildargv (arg); old_chain = make_cleanup_freeargv (argv); argc = 0; for (i = 0; argv[i]; i++) Index: symfile.c =================================================================== RCS file: /cvs/src/src/gdb/symfile.c,v retrieving revision 1.217 diff -u -p -u -r1.217 symfile.c --- symfile.c 1 Oct 2008 17:21:06 -0000 1.217 +++ symfile.c 3 Oct 2008 16:28:35 -0000 @@ -1483,14 +1483,11 @@ symbol_file_command (char *args, int fro } else { - char **argv = buildargv (args); + char **argv = gdb_buildargv (args); int flags = OBJF_USERLOADED; struct cleanup *cleanups; char *name = NULL; - if (argv == NULL) - nomem (0); - cleanups = make_cleanup_freeargv (argv); while (*argv != NULL) { @@ -1924,11 +1921,10 @@ generic_load (char *args, int from_tty) make_cleanup (clear_memory_write_data, &cbdata.requests); - argv = buildargv (args); - - if (argv == NULL) - nomem(0); + if (args == NULL) + error_no_arg (_("file to load")); + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); filename = tilde_expand (argv[0]); @@ -2117,12 +2113,9 @@ add_symbol_file_command (char *args, int if (args == NULL) error (_("add-symbol-file takes a file name and an address")); - argv = buildargv (args); + argv = gdb_buildargv (args); make_cleanup_freeargv (argv); - if (argv == NULL) - nomem (0); - for (arg = argv[0], argcnt = 0; arg != NULL; arg = argv[++argcnt]) { /* Process the argument. */ Index: symmisc.c =================================================================== RCS file: /cvs/src/src/gdb/symmisc.c,v retrieving revision 1.57 diff -u -p -u -r1.57 symmisc.c --- symmisc.c 1 Oct 2008 16:56:52 -0000 1.57 +++ symmisc.c 3 Oct 2008 16:28:35 -0000 @@ -526,10 +526,7 @@ maintenance_print_symbols (char *args, i error (_("\ Arguments missing: an output file name and an optional symbol file name")); } - else if ((argv = buildargv (args)) == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); cleanups = make_cleanup_freeargv (argv); if (argv[0] != NULL) @@ -739,10 +736,7 @@ maintenance_print_psymbols (char *args, { error (_("print-psymbols takes an output file name and optional symbol file name")); } - else if ((argv = buildargv (args)) == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); cleanups = make_cleanup_freeargv (argv); if (argv[0] != NULL) @@ -878,10 +872,7 @@ maintenance_print_msymbols (char *args, { error (_("print-msymbols takes an output file name and optional symbol file name")); } - else if ((argv = buildargv (args)) == NULL) - { - nomem (0); - } + argv = gdb_buildargv (args); cleanups = make_cleanup_freeargv (argv); if (argv[0] != NULL) Index: utils.c =================================================================== RCS file: /cvs/src/src/gdb/utils.c,v retrieving revision 1.195 diff -u -p -u -r1.195 utils.c --- utils.c 8 Sep 2008 21:57:42 -0000 1.195 +++ utils.c 3 Oct 2008 16:28:35 -0000 @@ -3349,3 +3349,17 @@ ldirname (const char *filename) dirname[base - filename] = '\0'; return dirname; } + +/* Call libiberty's buildargv, and return the result. + If buildargv fails due to out-of-memory, call nomem. + Therefore, the returned value is guaranteed to be non-NULL, + unless the parameter itself is NULL. */ + +char ** +gdb_buildargv (const char *s) +{ + char **argv = buildargv (s); + if (s != NULL && argv == NULL) + nomem (0); + return argv; +} ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-10-03 16:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-09-16 17:50 GDB aborts on missing command args. Which way to fix? Paul Pluzhnikov 2008-09-19 21:52 ` Tom Tromey 2008-09-19 22:02 ` Daniel Jacobowitz 2008-09-26 23:22 ` [patch] " Paul Pluzhnikov 2008-10-03 7:04 ` Joel Brobecker 2008-10-03 16:39 ` Paul Pluzhnikov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox