* [patch] PID cmdline argument should be whole from digits
@ 2010-02-13 15:20 Jan Kratochvil
2010-02-13 17:47 ` Eli Zaretskii
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kratochvil @ 2010-02-13 15:20 UTC (permalink / raw)
To: gdb-patches
Hi,
# gdb ./1 1.core
Attaching to program: .../1, process 1
warning: process 1 is a cloned process
Cannot access memory at address 0x3d001006
that is scary because on a test box I ran as root. Normally it just prints
(and loads it as a core file then):
Attaching to program: .../1, process 1
ptrace: Operation not permitted.
No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.
strspn is already freely in use by libiberty/cplus-dem.c.
I will check it in as [obv] in several days if there are no objections.
Thanks,
Jan
2010-02-13 Jan Kratochvil <jan.kratochvil@redhat.com>
* main.c (captured_main) <pid_or_core_arg>: Check the whole
PID_OR_CORE_ARG consists of digit characters.
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -827,10 +827,11 @@ Can't attach to process and specify a core file at the same time."));
else if (pid_or_core_arg)
{
/* The user specified 'gdb program pid' or gdb program core'.
- If pid_or_core_arg's first character is a digit, try attach
- first and then corefile. Otherwise try just corefile. */
+ If pid_or_core_arg's is a number, try attach first and then corefile.
+ Otherwise try just corefile. */
- if (isdigit (pid_or_core_arg[0]))
+ if (pid_or_core_arg[0] != 0
+ && strspn (pid_or_core_arg, "0123456789") == strlen (pid_or_core_arg))
{
if (catch_command_errors (attach_command, pid_or_core_arg,
!batch, RETURN_MASK_ALL) == 0)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [patch] PID cmdline argument should be whole from digits 2010-02-13 15:20 [patch] PID cmdline argument should be whole from digits Jan Kratochvil @ 2010-02-13 17:47 ` Eli Zaretskii 2010-02-13 21:03 ` Jan Kratochvil 0 siblings, 1 reply; 10+ messages in thread From: Eli Zaretskii @ 2010-02-13 17:47 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches > Date: Sat, 13 Feb 2010 16:20:44 +0100 > From: Jan Kratochvil <jan.kratochvil@redhat.com> > > + if (pid_or_core_arg[0] != 0 > + && strspn (pid_or_core_arg, "0123456789") == strlen (pid_or_core_arg)) Should we disallow PIDs in hex? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] PID cmdline argument should be whole from digits 2010-02-13 17:47 ` Eli Zaretskii @ 2010-02-13 21:03 ` Jan Kratochvil 2010-02-14 14:37 ` [patch] PID cmdline argument should be whole from digits [+testcase] Jan Kratochvil 0 siblings, 1 reply; 10+ messages in thread From: Jan Kratochvil @ 2010-02-13 21:03 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches On Sat, 13 Feb 2010 18:44:54 +0100, Eli Zaretskii wrote: > > Date: Sat, 13 Feb 2010 16:20:44 +0100 > > From: Jan Kratochvil <jan.kratochvil@redhat.com> > > > > + if (pid_or_core_arg[0] != 0 > > + && strspn (pid_or_core_arg, "0123456789") == strlen (pid_or_core_arg)) > > Should we disallow PIDs in hex? Thanks for the notice, the patch was probably more wrong as the PID interpretation should be more left on the target's interpretation. Attached patch no longer tries to attach although it will print there: Illegal process-id: 1.core. No problem dropping the patch, I thought it will be easy, it is not. Verified Microsoft Visual Studio 2010 (beta?) supports neither of getpid, __getpid nor pid_t. No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu. Thanks, Jan gdb/ 2010-02-13 Jan Kratochvil <jan.kratochvil@redhat.com> * configure.ac (AC_CHECK_FUNCS): Check for getpid. * configure: Regenerate. * config.in: Regenerate. * defs.h (parse_pid): New. * utils.c (parse_pid): New. * darwin-nat.c (darwin_attach): Replace ARGS parsing by parse_pid. * gnu-nat.c (gnu_attach): Likewise. * nto-procfs.c (procfs_attach): Likewise. * procfs.c (procfs_attach): Likewise. * windows-nat.c (windows_attach): Likewise. * inf-ptrace.c (inf_ptrace_attach): Likewise. Remove variable dummy. * inf-ttrace.c (inf_ttrace_attach): Likewise. * remote.c (extended_remote_attach_1): Likewise. gdb/testsuite/ 2010-02-13 Jan Kratochvil <jan.kratochvil@redhat.com> * gdb.base/default.exp (attach): Update the expect string. --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -799,7 +799,7 @@ AC_FUNC_ALLOCA AC_FUNC_MMAP AC_FUNC_VFORK AC_CHECK_FUNCS([canonicalize_file_name realpath getrusage getuid \ - getgid pipe poll pread64 sbrk setpgid setpgrp setsid \ + getpid getgid pipe poll pread64 sbrk setpgid setpgrp setsid \ sigaction sigprocmask sigsetmask socketpair syscall \ ttrace wborder setlocale iconvlist libiconvlist btowc \ setrlimit getrlimit]) --- a/gdb/darwin-nat.c +++ b/gdb/darwin-nat.c @@ -1514,13 +1514,7 @@ darwin_attach (struct target_ops *ops, char *args, int from_tty) struct inferior *inf; kern_return_t kret; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); - - if (pid == getpid ()) /* Trying to masturbate? */ - error (_("I refuse to debug myself!")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/defs.h +++ b/gdb/defs.h @@ -426,6 +426,8 @@ int compare_positive_ints (const void *ap, const void *bp); extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching); +extern unsigned long parse_pid (char *args); + /* From demangle.c */ extern void set_demangling_style (char *); --- a/gdb/gnu-nat.c +++ b/gdb/gnu-nat.c @@ -2139,13 +2139,7 @@ gnu_attach (struct target_ops *ops, char *args, int from_tty) struct inf *inf = cur_inf (); struct inferior *inferior; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); - - if (pid == getpid ()) /* Trying to masturbate? */ - error (_("I refuse to debug myself!")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/inf-ptrace.c +++ b/gdb/inf-ptrace.c @@ -187,20 +187,9 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty) { char *exec_file; pid_t pid; - char *dummy; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - /* Some targets don't set errno on errors, grrr! */ - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); - - if (pid == getpid ()) /* Trying to masturbate? */ - error (_("I refuse to debug myself!")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/inf-ttrace.c +++ b/gdb/inf-ttrace.c @@ -691,20 +691,10 @@ inf_ttrace_attach (struct target_ops *ops, char *args, int from_tty) { char *exec_file; pid_t pid; - char *dummy; ttevent_t tte; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); - - if (pid == getpid ()) /* Trying to masturbate? */ - error (_("I refuse to debug myself!")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/nto-procfs.c +++ b/gdb/nto-procfs.c @@ -619,13 +619,7 @@ procfs_attach (struct target_ops *ops, char *args, int from_tty) int pid; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); - - if (pid == getpid ()) - error (_("Attaching GDB to itself is not a good idea...")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -3606,12 +3606,7 @@ procfs_attach (struct target_ops *ops, char *args, int from_tty) char *exec_file; int pid; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); - if (pid == getpid ()) - error (_("Attaching GDB to itself is not a good idea...")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/remote.c +++ b/gdb/remote.c @@ -3857,17 +3857,9 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty) { struct remote_state *rs = get_remote_state (); int pid; - char *dummy; char *wait_status = NULL; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - /* Some targets don't set errno on errors, grrr! */ - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); + pid = parse_pid (args); if (remote_protocol_packets[PACKET_vAttach].support == PACKET_DISABLE) error (_("This target does not support attaching to a process")); --- a/gdb/utils.c +++ b/gdb/utils.c @@ -3648,6 +3648,32 @@ gdb_bfd_errmsg (bfd_error_type error_tag, char **matching) return ret; } +/* Return ARGS parsed as numeric pid. Call error if ARGS is not a valid number + or if ARGS is GDB's GETPID. */ + +unsigned long +parse_pid (char *args) +{ + unsigned long pid; + char *dummy; + + if (!args) + error_no_arg (_("process-id")); + + dummy = args; + pid = strtoul (args, &dummy, 0); + /* Some targets don't set errno on errors, grrr! */ + if ((pid == 0 && dummy == args) || dummy != &args[strlen (args)]) + error (_("Illegal process-id: %s."), args); + +#ifdef HAVE_GETPID + if (pid == getpid ()) /* Trying to masturbate? */ + error (_("I refuse to debug myself!")); +#endif + + return pid; +} + /* Provide a prototype to silence -Wmissing-prototypes. */ extern initialize_file_ftype _initialize_utils; --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -1691,8 +1691,7 @@ windows_attach (struct target_ops *ops, char *args, int from_tty) BOOL ok; DWORD pid; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid (args); if (set_process_privilege (SE_DEBUG_NAME, TRUE) < 0) { @@ -1700,8 +1699,6 @@ windows_attach (struct target_ops *ops, char *args, int from_tty) printf_unfiltered ("This can cause attach to fail on Windows NT/2K/XP\n"); } - pid = strtoul (args, 0, 0); /* Windows pid */ - windows_init_thread_list (); ok = DebugActiveProcess (pid); saw_create = 0; --- a/gdb/testsuite/gdb.base/default.exp +++ b/gdb/testsuite/gdb.base/default.exp @@ -42,7 +42,7 @@ gdb_test "append binary value" "Missing filename\." setup_xfail "mips-idt-*" send_gdb "attach\n" gdb_expect { - -re "Argument required .(process-id|program) to attach.*$gdb_prompt $"\ + -re "Argument required .(process-id|program to attach).*$gdb_prompt $"\ { pass "attach" } -re "You can't do that when your target is `None'.*$gdb_prompt $"\ { pass "attach" } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] PID cmdline argument should be whole from digits [+testcase] 2010-02-13 21:03 ` Jan Kratochvil @ 2010-02-14 14:37 ` Jan Kratochvil 2010-02-14 14:54 ` Pedro Alves 0 siblings, 1 reply; 10+ messages in thread From: Jan Kratochvil @ 2010-02-14 14:37 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches Hi, (+testcase as it is not so complicated to extend the existing one) Tested on x86_64-fedora12-linux-gnu. Regards, Jan gdb/ 2010-02-13 Jan Kratochvil <jan.kratochvil@redhat.com> * configure.ac (AC_CHECK_FUNCS): Check for getpid. * configure: Regenerate. * config.in: Regenerate. * defs.h (parse_pid): New. * utils.c (parse_pid): New. * darwin-nat.c (darwin_attach): Replace ARGS parsing by parse_pid. * gnu-nat.c (gnu_attach): Likewise. * nto-procfs.c (procfs_attach): Likewise. * procfs.c (procfs_attach): Likewise. * windows-nat.c (windows_attach): Likewise. * inf-ptrace.c (inf_ptrace_attach): Likewise. Remove variable dummy. * inf-ttrace.c (inf_ttrace_attach): Likewise. * remote.c (extended_remote_attach_1): Likewise. gdb/testsuite/ 2010-02-13 Jan Kratochvil <jan.kratochvil@redhat.com> * gdb.base/default.exp (attach): Update the expect string. * gdb.base/attach.exp (attach to nonsense is prohibited): Make the "Illegal process-id" expect string more exact. (attach to digits-starting nonsense is prohibited): New. --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -799,7 +799,7 @@ AC_FUNC_ALLOCA AC_FUNC_MMAP AC_FUNC_VFORK AC_CHECK_FUNCS([canonicalize_file_name realpath getrusage getuid \ - getgid pipe poll pread64 sbrk setpgid setpgrp setsid \ + getpid getgid pipe poll pread64 sbrk setpgid setpgrp setsid \ sigaction sigprocmask sigsetmask socketpair syscall \ ttrace wborder setlocale iconvlist libiconvlist btowc \ setrlimit getrlimit]) --- a/gdb/darwin-nat.c +++ b/gdb/darwin-nat.c @@ -1514,13 +1514,7 @@ darwin_attach (struct target_ops *ops, char *args, int from_tty) struct inferior *inf; kern_return_t kret; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); - - if (pid == getpid ()) /* Trying to masturbate? */ - error (_("I refuse to debug myself!")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/defs.h +++ b/gdb/defs.h @@ -426,6 +426,8 @@ int compare_positive_ints (const void *ap, const void *bp); extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching); +extern unsigned long parse_pid (char *args); + /* From demangle.c */ extern void set_demangling_style (char *); --- a/gdb/gnu-nat.c +++ b/gdb/gnu-nat.c @@ -2139,13 +2139,7 @@ gnu_attach (struct target_ops *ops, char *args, int from_tty) struct inf *inf = cur_inf (); struct inferior *inferior; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); - - if (pid == getpid ()) /* Trying to masturbate? */ - error (_("I refuse to debug myself!")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/inf-ptrace.c +++ b/gdb/inf-ptrace.c @@ -187,20 +187,9 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty) { char *exec_file; pid_t pid; - char *dummy; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - /* Some targets don't set errno on errors, grrr! */ - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); - - if (pid == getpid ()) /* Trying to masturbate? */ - error (_("I refuse to debug myself!")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/inf-ttrace.c +++ b/gdb/inf-ttrace.c @@ -691,20 +691,10 @@ inf_ttrace_attach (struct target_ops *ops, char *args, int from_tty) { char *exec_file; pid_t pid; - char *dummy; ttevent_t tte; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); - - if (pid == getpid ()) /* Trying to masturbate? */ - error (_("I refuse to debug myself!")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/nto-procfs.c +++ b/gdb/nto-procfs.c @@ -619,13 +619,7 @@ procfs_attach (struct target_ops *ops, char *args, int from_tty) int pid; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); - - if (pid == getpid ()) - error (_("Attaching GDB to itself is not a good idea...")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -3606,12 +3606,7 @@ procfs_attach (struct target_ops *ops, char *args, int from_tty) char *exec_file; int pid; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); - if (pid == getpid ()) - error (_("Attaching GDB to itself is not a good idea...")); + pid = parse_pid (args); if (from_tty) { --- a/gdb/remote.c +++ b/gdb/remote.c @@ -3857,17 +3857,9 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty) { struct remote_state *rs = get_remote_state (); int pid; - char *dummy; char *wait_status = NULL; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - /* Some targets don't set errno on errors, grrr! */ - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); + pid = parse_pid (args); if (remote_protocol_packets[PACKET_vAttach].support == PACKET_DISABLE) error (_("This target does not support attaching to a process")); --- a/gdb/utils.c +++ b/gdb/utils.c @@ -3648,6 +3648,32 @@ gdb_bfd_errmsg (bfd_error_type error_tag, char **matching) return ret; } +/* Return ARGS parsed as numeric pid. Call error if ARGS is not a valid number + or if ARGS is GDB's GETPID. */ + +unsigned long +parse_pid (char *args) +{ + unsigned long pid; + char *dummy; + + if (!args) + error_no_arg (_("process-id")); + + dummy = args; + pid = strtoul (args, &dummy, 0); + /* Some targets don't set errno on errors, grrr! */ + if ((pid == 0 && dummy == args) || dummy != &args[strlen (args)]) + error (_("Illegal process-id: %s."), args); + +#ifdef HAVE_GETPID + if (pid == getpid ()) /* Trying to masturbate? */ + error (_("I refuse to debug myself!")); +#endif + + return pid; +} + /* Provide a prototype to silence -Wmissing-prototypes. */ extern initialize_file_ftype _initialize_utils; --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -1691,8 +1691,7 @@ windows_attach (struct target_ops *ops, char *args, int from_tty) BOOL ok; DWORD pid; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid (args); if (set_process_privilege (SE_DEBUG_NAME, TRUE) < 0) { @@ -1700,8 +1699,6 @@ windows_attach (struct target_ops *ops, char *args, int from_tty) printf_unfiltered ("This can cause attach to fail on Windows NT/2K/XP\n"); } - pid = strtoul (args, 0, 0); /* Windows pid */ - windows_init_thread_list (); ok = DebugActiveProcess (pid); saw_create = 0; --- a/gdb/testsuite/gdb.base/default.exp +++ b/gdb/testsuite/gdb.base/default.exp @@ -42,7 +42,7 @@ gdb_test "append binary value" "Missing filename\." setup_xfail "mips-idt-*" send_gdb "attach\n" gdb_expect { - -re "Argument required .(process-id|program) to attach.*$gdb_prompt $"\ + -re "Argument required .(process-id|program to attach).*$gdb_prompt $"\ { pass "attach" } -re "You can't do that when your target is `None'.*$gdb_prompt $"\ { pass "attach" } --- a/gdb/testsuite/gdb.base/attach.exp +++ b/gdb/testsuite/gdb.base/attach.exp @@ -93,7 +93,28 @@ proc do_attach_tests {} { set test "attach to nonsense is prohibited" gdb_test_multiple "attach abc" "$test" { - -re "Illegal process-id: abc.*$gdb_prompt $" { + -re "Illegal process-id: abc\\.\r\n$gdb_prompt $" { + pass "$test" + } + -re "Attaching to.*, process .*couldn't open /proc file.*$gdb_prompt $" { + # Response expected from /proc-based systems. + pass "$test" + } + -re "Can't attach to process..*$gdb_prompt $" { + # Response expected on Cygwin + pass "$test" + } + -re "Attaching to.*$gdb_prompt $" { + fail "$test (bogus pid allowed)" + } + } + + # Verify that we cannot attach to nonsense even if its initial part is + # a valid PID. + + set test "attach to digits-starting nonsense is prohibited" + gdb_test_multiple "attach ${testpid}x" "$test" { + -re "Illegal process-id: ${testpid}x\\.\r\n$gdb_prompt $" { pass "$test" } -re "Attaching to.*, process .*couldn't open /proc file.*$gdb_prompt $" { ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] PID cmdline argument should be whole from digits [+testcase] 2010-02-14 14:37 ` [patch] PID cmdline argument should be whole from digits [+testcase] Jan Kratochvil @ 2010-02-14 14:54 ` Pedro Alves 2010-02-14 18:56 ` Jan Kratochvil 0 siblings, 1 reply; 10+ messages in thread From: Pedro Alves @ 2010-02-14 14:54 UTC (permalink / raw) To: gdb-patches; +Cc: Jan Kratochvil, Eli Zaretskii On Sunday 14 February 2010 14:37:16, Jan Kratochvil wrote: > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -3857,17 +3857,9 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty) > { > struct remote_state *rs = get_remote_state (); > int pid; > - char *dummy; > char *wait_status = NULL; > > - if (!args) > - error_no_arg (_("process-id to attach")); > - > - dummy = args; > - pid = strtol (args, &dummy, 0); > - /* Some targets don't set errno on errors, grrr! */ > - if (pid == 0 && args == dummy) > - error (_("Illegal process-id: %s."), args); > + pid = parse_pid (args); > +unsigned long > +parse_pid (char *args) > +{ (...) > +#ifdef HAVE_GETPID > + if (pid == getpid ()) /* Trying to masturbate? */ > + error (_("I refuse to debug myself!")); > +#endif (...) Certainly a remote PID has nothing to do with a local PID. On windows-nat.c, the PID can either be a Windows or Cygwin PID. -- Pedro Alves ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] PID cmdline argument should be whole from digits [+testcase] 2010-02-14 14:54 ` Pedro Alves @ 2010-02-14 18:56 ` Jan Kratochvil 2010-02-15 14:57 ` Pedro Alves 0 siblings, 1 reply; 10+ messages in thread From: Jan Kratochvil @ 2010-02-14 18:56 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Eli Zaretskii On Sun, 14 Feb 2010 15:54:10 +0100, Pedro Alves wrote: > Certainly a remote PID has nothing to do with a local PID. OK, I am sorry. > On windows-nat.c, the PID can either be a Windows or Cygwin PID. On non-cygwin (mingw?) HAVE_GETPID should be false and in Cygwin it should be compatible. But I understand relying on HAVE_* being false is not good. (moved getpid checking out of parse_pid* + kept "process-id to attach" message intact) parse_pid_to_attach does not use pid_t return type as while it is defined-as-needed by config.h it would not be compatible with windows-nat.c expected DWORD type. No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu. Thanks, Jan gdb/ 2010-02-14 Jan Kratochvil <jan.kratochvil@redhat.com> * defs.h (parse_pid_to_attach): New. * utils.c (parse_pid_to_attach): New. * darwin-nat.c (darwin_attach): Replace ARGS parsing by parse_pid. * gnu-nat.c (gnu_attach): Likewise. * nto-procfs.c (procfs_attach): Likewise. * procfs.c (procfs_attach): Likewise. * windows-nat.c (windows_attach): Likewise. * inf-ptrace.c (inf_ptrace_attach): Likewise. Remove variable dummy. * inf-ttrace.c (inf_ttrace_attach): Likewise. * remote.c (extended_remote_attach_1): Likewise. New comment on getpid check. gdb/testsuite/ 2010-02-14 Jan Kratochvil <jan.kratochvil@redhat.com> * gdb.base/attach.exp (attach to nonsense is prohibited): Make the "Illegal process-id" expect string more exact. (attach to digits-starting nonsense is prohibited): New. --- a/gdb/darwin-nat.c +++ b/gdb/darwin-nat.c @@ -1514,12 +1514,9 @@ darwin_attach (struct target_ops *ops, char *args, int from_tty) struct inferior *inf; kern_return_t kret; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid_to_attach (args); - pid = atoi (args); - - if (pid == getpid ()) /* Trying to masturbate? */ + if (pid == getpid ()) /* Trying to masturbate? */ error (_("I refuse to debug myself!")); if (from_tty) --- a/gdb/defs.h +++ b/gdb/defs.h @@ -426,6 +426,8 @@ int compare_positive_ints (const void *ap, const void *bp); extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching); +extern unsigned long parse_pid_to_attach (char *args); + /* From demangle.c */ extern void set_demangling_style (char *); --- a/gdb/gnu-nat.c +++ b/gdb/gnu-nat.c @@ -2139,12 +2139,9 @@ gnu_attach (struct target_ops *ops, char *args, int from_tty) struct inf *inf = cur_inf (); struct inferior *inferior; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); + pid = parse_pid_to_attach (args); - if (pid == getpid ()) /* Trying to masturbate? */ + if (pid == getpid ()) /* Trying to masturbate? */ error (_("I refuse to debug myself!")); if (from_tty) --- a/gdb/inf-ptrace.c +++ b/gdb/inf-ptrace.c @@ -187,17 +187,9 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty) { char *exec_file; pid_t pid; - char *dummy; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - /* Some targets don't set errno on errors, grrr! */ - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); + pid = parse_pid_to_attach (args); if (pid == getpid ()) /* Trying to masturbate? */ error (_("I refuse to debug myself!")); --- a/gdb/inf-ttrace.c +++ b/gdb/inf-ttrace.c @@ -691,17 +691,10 @@ inf_ttrace_attach (struct target_ops *ops, char *args, int from_tty) { char *exec_file; pid_t pid; - char *dummy; ttevent_t tte; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); + pid = parse_pid_to_attach (args); if (pid == getpid ()) /* Trying to masturbate? */ error (_("I refuse to debug myself!")); --- a/gdb/nto-procfs.c +++ b/gdb/nto-procfs.c @@ -619,10 +619,7 @@ procfs_attach (struct target_ops *ops, char *args, int from_tty) int pid; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); + pid = parse_pid_to_attach (args); if (pid == getpid ()) error (_("Attaching GDB to itself is not a good idea...")); --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -3606,10 +3606,8 @@ procfs_attach (struct target_ops *ops, char *args, int from_tty) char *exec_file; int pid; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid_to_attach (args); - pid = atoi (args); if (pid == getpid ()) error (_("Attaching GDB to itself is not a good idea...")); --- a/gdb/remote.c +++ b/gdb/remote.c @@ -3857,17 +3857,12 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty) { struct remote_state *rs = get_remote_state (); int pid; - char *dummy; char *wait_status = NULL; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid_to_attach (args); - dummy = args; - pid = strtol (args, &dummy, 0); - /* Some targets don't set errno on errors, grrr! */ - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); + /* Remote PID can be freely equal to getpid, do not check it here the same + way as in other targets. */ if (remote_protocol_packets[PACKET_vAttach].support == PACKET_DISABLE) error (_("This target does not support attaching to a process")); --- a/gdb/testsuite/gdb.base/attach.exp +++ b/gdb/testsuite/gdb.base/attach.exp @@ -93,7 +93,28 @@ proc do_attach_tests {} { set test "attach to nonsense is prohibited" gdb_test_multiple "attach abc" "$test" { - -re "Illegal process-id: abc.*$gdb_prompt $" { + -re "Illegal process-id: abc\\.\r\n$gdb_prompt $" { + pass "$test" + } + -re "Attaching to.*, process .*couldn't open /proc file.*$gdb_prompt $" { + # Response expected from /proc-based systems. + pass "$test" + } + -re "Can't attach to process..*$gdb_prompt $" { + # Response expected on Cygwin + pass "$test" + } + -re "Attaching to.*$gdb_prompt $" { + fail "$test (bogus pid allowed)" + } + } + + # Verify that we cannot attach to nonsense even if its initial part is + # a valid PID. + + set test "attach to digits-starting nonsense is prohibited" + gdb_test_multiple "attach ${testpid}x" "$test" { + -re "Illegal process-id: ${testpid}x\\.\r\n$gdb_prompt $" { pass "$test" } -re "Attaching to.*, process .*couldn't open /proc file.*$gdb_prompt $" { --- a/gdb/utils.c +++ b/gdb/utils.c @@ -3648,6 +3648,26 @@ gdb_bfd_errmsg (bfd_error_type error_tag, char **matching) return ret; } +/* Return ARGS parsed as numeric pid. */ + +unsigned long +parse_pid_to_attach (char *args) +{ + unsigned long pid; + char *dummy; + + if (!args) + error_no_arg (_("process-id to attach")); + + dummy = args; + pid = strtoul (args, &dummy, 0); + /* Some targets don't set errno on errors, grrr! */ + if ((pid == 0 && dummy == args) || dummy != &args[strlen (args)]) + error (_("Illegal process-id: %s."), args); + + return pid; +} + /* Provide a prototype to silence -Wmissing-prototypes. */ extern initialize_file_ftype _initialize_utils; --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -1691,8 +1691,7 @@ windows_attach (struct target_ops *ops, char *args, int from_tty) BOOL ok; DWORD pid; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid_to_attach (args); if (set_process_privilege (SE_DEBUG_NAME, TRUE) < 0) { @@ -1700,8 +1699,6 @@ windows_attach (struct target_ops *ops, char *args, int from_tty) printf_unfiltered ("This can cause attach to fail on Windows NT/2K/XP\n"); } - pid = strtoul (args, 0, 0); /* Windows pid */ - windows_init_thread_list (); ok = DebugActiveProcess (pid); saw_create = 0; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] PID cmdline argument should be whole from digits [+testcase] 2010-02-14 18:56 ` Jan Kratochvil @ 2010-02-15 14:57 ` Pedro Alves 2010-02-15 15:52 ` Jan Kratochvil 0 siblings, 1 reply; 10+ messages in thread From: Pedro Alves @ 2010-02-15 14:57 UTC (permalink / raw) To: Jan Kratochvil; +Cc: gdb-patches, Eli Zaretskii On Sunday 14 February 2010 18:56:04, Jan Kratochvil wrote: > On Sun, 14 Feb 2010 15:54:10 +0100, Pedro Alves wrote: > > On windows-nat.c, the PID can either be a Windows or Cygwin PID. > > On non-cygwin (mingw?) HAVE_GETPID should be false and in Cygwin it should be > compatible. mingw does have getpid/_getpid. What I meant was for you to notice that windows-nat.c when built for Cygwin tries PID as a Windows PID, and if that fails, tries PID as a Cygwin PID. Probably doesn't matter in this case; but this further demonstrates on top of the remote example, that there's target side tweakable behaviour here. > But I understand relying on HAVE_* being false is not good. Yes. > > (moved getpid checking out of parse_pid* + kept "process-id to attach" message > intact) > > parse_pid_to_attach does not use pid_t return type as while it is > defined-as-needed by config.h it would not be compatible with windows-nat.c > expected DWORD type. (pid_t would only make sense for native targets anyway. At some point, we may have to come up with a target_pid_t, or some such, but we've dodged it so far.) I'd prefer to make it return int, because it's what we in use common code to store a pid. E.g., ptid_t.pid, inferior->pid. Note that DWORD is always 32-bit, even on 64-bit Windows. Can you extend the describing comment of parse_pid_to_attach, with something like: Either returns a valid PID, or throws an error. Otherwise, looks OK to me. -- Pedro Alves ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] PID cmdline argument should be whole from digits [+testcase] 2010-02-15 14:57 ` Pedro Alves @ 2010-02-15 15:52 ` Jan Kratochvil 2010-02-15 16:03 ` Mark Kettenis 2010-02-15 17:38 ` Jan Kratochvil 0 siblings, 2 replies; 10+ messages in thread From: Jan Kratochvil @ 2010-02-15 15:52 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Eli Zaretskii On Mon, 15 Feb 2010 15:57:24 +0100, Pedro Alves wrote: > mingw does have getpid/_getpid. What I meant was for you to notice > that windows-nat.c when built for Cygwin tries PID as a Windows PID, > and if that fails, tries PID as a Cygwin PID. Probably doesn't matter > in this case; but this further demonstrates on top of the remote > example, that there's target side tweakable behaviour here. OK. Not sure if the getpid() check should have been there or not but it is a change outside of the scope of this patch to be decided by MS-Windows target maintainers. > I'd prefer to make it return int, because it's what we in > use common code to store a pid. E.g., ptid_t.pid, OK, good point. > Note that DWORD is always 32-bit, even on 64-bit Windows. OK, did not expect, "DWORD" looked large. > Can you extend the describing comment of parse_pid_to_attach, > with something like: > > Either returns a valid PID, or throws an error. /* Return ARGS parsed as a valid pid, or throw an error. */ > Otherwise, looks OK to me. Considering thus as approved, I will check it in. Thanks, Jan gdb/ 2010-02-15 Jan Kratochvil <jan.kratochvil@redhat.com> * defs.h (parse_pid_to_attach): New. * utils.c (parse_pid_to_attach): New. * darwin-nat.c (darwin_attach): Replace ARGS parsing by parse_pid. * gnu-nat.c (gnu_attach): Likewise. * nto-procfs.c (procfs_attach): Likewise. * procfs.c (procfs_attach): Likewise. * windows-nat.c (windows_attach): Likewise. * inf-ptrace.c (inf_ptrace_attach): Likewise. Remove variable dummy. * inf-ttrace.c (inf_ttrace_attach): Likewise. * remote.c (extended_remote_attach_1): Likewise. New comment on getpid check. gdb/testsuite/ 2010-02-15 Jan Kratochvil <jan.kratochvil@redhat.com> * gdb.base/attach.exp (attach to nonsense is prohibited): Make the "Illegal process-id" expect string more exact. (attach to digits-starting nonsense is prohibited): New. --- a/gdb/darwin-nat.c +++ b/gdb/darwin-nat.c @@ -1514,12 +1514,9 @@ darwin_attach (struct target_ops *ops, char *args, int from_tty) struct inferior *inf; kern_return_t kret; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid_to_attach (args); - pid = atoi (args); - - if (pid == getpid ()) /* Trying to masturbate? */ + if (pid == getpid ()) /* Trying to masturbate? */ error (_("I refuse to debug myself!")); if (from_tty) --- a/gdb/defs.h +++ b/gdb/defs.h @@ -426,6 +426,8 @@ int compare_positive_ints (const void *ap, const void *bp); extern const char *gdb_bfd_errmsg (bfd_error_type error_tag, char **matching); +extern int parse_pid_to_attach (char *args); + /* From demangle.c */ extern void set_demangling_style (char *); --- a/gdb/gnu-nat.c +++ b/gdb/gnu-nat.c @@ -2139,12 +2139,9 @@ gnu_attach (struct target_ops *ops, char *args, int from_tty) struct inf *inf = cur_inf (); struct inferior *inferior; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); + pid = parse_pid_to_attach (args); - if (pid == getpid ()) /* Trying to masturbate? */ + if (pid == getpid ()) /* Trying to masturbate? */ error (_("I refuse to debug myself!")); if (from_tty) --- a/gdb/inf-ptrace.c +++ b/gdb/inf-ptrace.c @@ -187,17 +187,9 @@ inf_ptrace_attach (struct target_ops *ops, char *args, int from_tty) { char *exec_file; pid_t pid; - char *dummy; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - /* Some targets don't set errno on errors, grrr! */ - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); + pid = parse_pid_to_attach (args); if (pid == getpid ()) /* Trying to masturbate? */ error (_("I refuse to debug myself!")); --- a/gdb/inf-ttrace.c +++ b/gdb/inf-ttrace.c @@ -691,17 +691,10 @@ inf_ttrace_attach (struct target_ops *ops, char *args, int from_tty) { char *exec_file; pid_t pid; - char *dummy; ttevent_t tte; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - dummy = args; - pid = strtol (args, &dummy, 0); - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); + pid = parse_pid_to_attach (args); if (pid == getpid ()) /* Trying to masturbate? */ error (_("I refuse to debug myself!")); --- a/gdb/nto-procfs.c +++ b/gdb/nto-procfs.c @@ -619,10 +619,7 @@ procfs_attach (struct target_ops *ops, char *args, int from_tty) int pid; struct inferior *inf; - if (!args) - error_no_arg (_("process-id to attach")); - - pid = atoi (args); + pid = parse_pid_to_attach (args); if (pid == getpid ()) error (_("Attaching GDB to itself is not a good idea...")); --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -3606,10 +3606,8 @@ procfs_attach (struct target_ops *ops, char *args, int from_tty) char *exec_file; int pid; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid_to_attach (args); - pid = atoi (args); if (pid == getpid ()) error (_("Attaching GDB to itself is not a good idea...")); --- a/gdb/remote.c +++ b/gdb/remote.c @@ -3857,17 +3857,12 @@ extended_remote_attach_1 (struct target_ops *target, char *args, int from_tty) { struct remote_state *rs = get_remote_state (); int pid; - char *dummy; char *wait_status = NULL; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid_to_attach (args); - dummy = args; - pid = strtol (args, &dummy, 0); - /* Some targets don't set errno on errors, grrr! */ - if (pid == 0 && args == dummy) - error (_("Illegal process-id: %s."), args); + /* Remote PID can be freely equal to getpid, do not check it here the same + way as in other targets. */ if (remote_protocol_packets[PACKET_vAttach].support == PACKET_DISABLE) error (_("This target does not support attaching to a process")); --- a/gdb/testsuite/gdb.base/attach.exp +++ b/gdb/testsuite/gdb.base/attach.exp @@ -93,7 +93,28 @@ proc do_attach_tests {} { set test "attach to nonsense is prohibited" gdb_test_multiple "attach abc" "$test" { - -re "Illegal process-id: abc.*$gdb_prompt $" { + -re "Illegal process-id: abc\\.\r\n$gdb_prompt $" { + pass "$test" + } + -re "Attaching to.*, process .*couldn't open /proc file.*$gdb_prompt $" { + # Response expected from /proc-based systems. + pass "$test" + } + -re "Can't attach to process..*$gdb_prompt $" { + # Response expected on Cygwin + pass "$test" + } + -re "Attaching to.*$gdb_prompt $" { + fail "$test (bogus pid allowed)" + } + } + + # Verify that we cannot attach to nonsense even if its initial part is + # a valid PID. + + set test "attach to digits-starting nonsense is prohibited" + gdb_test_multiple "attach ${testpid}x" "$test" { + -re "Illegal process-id: ${testpid}x\\.\r\n$gdb_prompt $" { pass "$test" } -re "Attaching to.*, process .*couldn't open /proc file.*$gdb_prompt $" { --- a/gdb/utils.c +++ b/gdb/utils.c @@ -3648,6 +3648,26 @@ gdb_bfd_errmsg (bfd_error_type error_tag, char **matching) return ret; } +/* Return ARGS parsed as a valid pid, or throw an error. */ + +int +parse_pid_to_attach (char *args) +{ + unsigned long pid; + char *dummy; + + if (!args) + error_no_arg (_("process-id to attach")); + + dummy = args; + pid = strtoul (args, &dummy, 0); + /* Some targets don't set errno on errors, grrr! */ + if ((pid == 0 && dummy == args) || dummy != &args[strlen (args)]) + error (_("Illegal process-id: %s."), args); + + return pid; +} + /* Provide a prototype to silence -Wmissing-prototypes. */ extern initialize_file_ftype _initialize_utils; --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -1691,8 +1691,7 @@ windows_attach (struct target_ops *ops, char *args, int from_tty) BOOL ok; DWORD pid; - if (!args) - error_no_arg (_("process-id to attach")); + pid = parse_pid_to_attach (args); if (set_process_privilege (SE_DEBUG_NAME, TRUE) < 0) { @@ -1700,8 +1699,6 @@ windows_attach (struct target_ops *ops, char *args, int from_tty) printf_unfiltered ("This can cause attach to fail on Windows NT/2K/XP\n"); } - pid = strtoul (args, 0, 0); /* Windows pid */ - windows_init_thread_list (); ok = DebugActiveProcess (pid); saw_create = 0; ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] PID cmdline argument should be whole from digits [+testcase] 2010-02-15 15:52 ` Jan Kratochvil @ 2010-02-15 16:03 ` Mark Kettenis 2010-02-15 17:38 ` Jan Kratochvil 1 sibling, 0 replies; 10+ messages in thread From: Mark Kettenis @ 2010-02-15 16:03 UTC (permalink / raw) To: jan.kratochvil; +Cc: pedro, gdb-patches, eliz > Date: Mon, 15 Feb 2010 16:52:03 +0100 > From: Jan Kratochvil <jan.kratochvil@redhat.com> > > > Note that DWORD is always 32-bit, even on 64-bit Windows. > > OK, did not expect, "DWORD" looked large. Microsoft still lives in the dark ages of 16-bit computing ;). Even on 64-bit Windows 'long' is only 32 bits. That alone is enough for me to not care about supporting Windows. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch] PID cmdline argument should be whole from digits [+testcase] 2010-02-15 15:52 ` Jan Kratochvil 2010-02-15 16:03 ` Mark Kettenis @ 2010-02-15 17:38 ` Jan Kratochvil 1 sibling, 0 replies; 10+ messages in thread From: Jan Kratochvil @ 2010-02-15 17:38 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Eli Zaretskii On Mon, 15 Feb 2010 16:52:03 +0100, Jan Kratochvil wrote: > Considering thus as approved, I will check it in. http://sourceware.org/ml/gdb-cvs/2010-02/msg00110.html Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-02-15 17:38 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-13 15:20 [patch] PID cmdline argument should be whole from digits Jan Kratochvil 2010-02-13 17:47 ` Eli Zaretskii 2010-02-13 21:03 ` Jan Kratochvil 2010-02-14 14:37 ` [patch] PID cmdline argument should be whole from digits [+testcase] Jan Kratochvil 2010-02-14 14:54 ` Pedro Alves 2010-02-14 18:56 ` Jan Kratochvil 2010-02-15 14:57 ` Pedro Alves 2010-02-15 15:52 ` Jan Kratochvil 2010-02-15 16:03 ` Mark Kettenis 2010-02-15 17:38 ` Jan Kratochvil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox