* 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