[patch] PID cmdline argument should be whole from digits

[patch] PID cmdline argument should be whole from digits

[Jan Kratochvil]

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)

Re: [patch] PID cmdline argument should be whole from digits

[Eli Zaretskii]

> 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?

Re: [patch] PID cmdline argument should be whole from digits

[Jan Kratochvil]

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" }

Re: [patch] PID cmdline argument should be whole from digits [+testcase]

[Jan Kratochvil]

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 $" {

Re: [patch] PID cmdline argument should be whole from digits [+testcase]

[Pedro Alves]

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

Re: [patch] PID cmdline argument should be whole from digits [+testcase]

[Jan Kratochvil]

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;

Re: [patch] PID cmdline argument should be whole from digits [+testcase]

[Pedro Alves]

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

Re: [patch] PID cmdline argument should be whole from digits [+testcase]

[Jan Kratochvil]

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;

Re: [patch] PID cmdline argument should be whole from digits [+testcase]

[Mark Kettenis]

> 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.

Re: [patch] PID cmdline argument should be whole from digits [+testcase]

[Jan Kratochvil]

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