gdbserver with reversed arguments goes into an infinite loop

gdbserver with reversed arguments goes into an infinite loop

[Denis PILAT]

I've found that if you revert the argument of gdbserver, means writing 
the program's name before the COMM argument, it goes into an infinite 
loop, and as the CTRL+C does not work, you have to kill the process from 
an other shell.

In gdbserver/server.c, the loop in question does the remote_open on the 
wrong passed argument (argv[1]) which unfortunately is the binary file 
you'd expect to open so remote_open does not exit on error.

I think either we could check that we pass correct argument before using 
start_inferior(), this is executing before the loop. The bellow patch is 
in that sense.

Or we find a way to exit the loop by adding a test in it. May be by 
adding something in remote_open to let it fail.


I'd like your opinion about that
Thanks
-- 
Denis Pilat / STMicroelectronics

Index: server.c
===================================================================
--- server.c    (revision 544)
+++ server.c    (working copy)
@@ -408,6 +408,13 @@ main (int argc, char *argv[])

   if (pid == 0)
     {
+      if (access (argv[2], F_OK) != 0)
+      {
+        fprintf (stderr, "File %s does not exist.\n",argv[2]);
+        gdbserver_usage ();
+        exit (0);
+      }
+
       /* Wait till we are at first instruction in program.  */
       signal = start_inferior (&argv[2], &status);

Re: gdbserver with reversed arguments goes into an infinite loop

[Daniel Jacobowitz]

On Wed, Dec 13, 2006 at 10:58:14AM +0100, Denis PILAT wrote:
> I've found that if you revert the argument of gdbserver, means writing 
> the program's name before the COMM argument, it goes into an infinite 
> loop, and as the CTRL+C does not work, you have to kill the process from 
> an other shell.
> 
> In gdbserver/server.c, the loop in question does the remote_open on the 
> wrong passed argument (argv[1]) which unfortunately is the binary file 
> you'd expect to open so remote_open does not exit on error.
> 
> I think either we could check that we pass correct argument before using 
> start_inferior(), this is executing before the loop. The bellow patch is 
> in that sense.
> 
> Or we find a way to exit the loop by adding a test in it. May be by 
> adding something in remote_open to let it fail.

I doubt it's in an infinite loop.  It's probably sleeping, "waiting"
for a connection.  We ought to allow C-c when no debugger is
connected yet.  It'd be nice if remote_open wouldn't open ordinary
files, too.

-- 
Daniel Jacobowitz
CodeSourcery

Re: gdbserver with reversed arguments goes into an infinite loop

[Denis PILAT]

Daniel Jacobowitz wrote:
> On Wed, Dec 13, 2006 at 10:58:14AM +0100, Denis PILAT wrote:
>> I've found that if you revert the argument of gdbserver, means writing 
>> the program's name before the COMM argument, it goes into an infinite 
>> loop, and as the CTRL+C does not work, you have to kill the process from 
>> an other shell.
>>
>> In gdbserver/server.c, the loop in question does the remote_open on the 
>> wrong passed argument (argv[1]) which unfortunately is the binary file 
>> you'd expect to open so remote_open does not exit on error.
>>
>> I think either we could check that we pass correct argument before using 
>> start_inferior(), this is executing before the loop. The bellow patch is 
>> in that sense.
>>
>> Or we find a way to exit the loop by adding a test in it. May be by 
>> adding something in remote_open to let it fail.
>
> I doubt it's in an infinite loop.  It's probably sleeping, "waiting"
> for a connection.  We ought to allow C-c when no debugger is
> connected yet.  It'd be nice if remote_open wouldn't open ordinary
> files, too.
>
Yes it's waiting for a connection but as arguments are wrongs it can 
wait for long ...

You're right, preventing remote_open from opening ordinary files would 
exit the loop.
But may be it would be better to open only character device (S_ISCHR 
macro) than excluding ordinary files (S_ISREG macro). It's up to you !
Here is a patch where I made a test to open only character device. If 
you right with that solution I'll propose a patch with a ChangeLog and 
so on.
I took this opportunity to remove a warning on a strncpy() usage.

I'm wondering about the compilation of this code under windows. I never 
compiled a gdbserver on windows, is there any gdbserver hosted under 
windows ?

-- 
Denis

Index: remote-utils.c
===================================================================
--- remote-utils.c      (revision 544)
+++ remote-utils.c      (working copy)
@@ -36,6 +36,7 @@
 #include <sys/time.h>
 #include <unistd.h>
 #include <arpa/inet.h>
+#include <sys/stat.h>

 #ifndef HAVE_SOCKLEN_T
 typedef int socklen_t;
@@ -68,10 +69,24 @@ void
 remote_open (char *name)
 {
   int save_fcntl_flags;
-
-  if (!strchr (name, ':'))
+  char *port_str;
+
+  port_str = strchr (name, ':');
+
+  /* if name is not of kind "HOST:PORT" it must be tty device.  */
+  if (!port_str)
     {
-      remote_desc = open (name, O_RDWR);
+      struct stat status;
+      int stat_result;
+      remote_desc = -1;
+
+      /* Open only character device.  */
+      stat_result = stat(name, &status);
+      if (!stat_result && S_ISCHR(status.st_mode))
+      {
+        remote_desc = open (name, O_RDWR);
+      }
+
       if (remote_desc < 0)
        perror_with_name ("Could not open remote device");

@@ -123,14 +138,11 @@ remote_open (char *name)
     }
   else
     {
-      char *port_str;
       int port;
       struct sockaddr_in sockaddr;
       socklen_t tmp;
       int tmp_desc;

-      port_str = strchr (name, ':');
-
       port = atoi (port_str + 1);

       tmp_desc = socket (PF_INET, SOCK_STREAM, 0);
@@ -650,7 +662,7 @@ prepare_resume_reply (char *buf, char st
          CORE_ADDR addr;
          int i;

-         strncpy (buf, "watch:", 6);
+         buf = strncpy (buf, "watch:", 6);
          buf += 6;

          addr = (*the_target->stopped_data_address) ();

Re: gdbserver with reversed arguments goes into an infinite loop

[Daniel Jacobowitz]

On Wed, Dec 13, 2006 at 04:45:53PM +0100, Denis PILAT wrote:
> But may be it would be better to open only character device (S_ISCHR 
> macro) than excluding ordinary files (S_ISREG macro). It's up to you !

Good idea.  FIFOs are OK too.  I've committed a nicer version of this.

> I took this opportunity to remove a warning on a strncpy() usage.

Why did it warn?  I omitted this bit, because I don't see any reason
(or any warning).

> I'm wondering about the compilation of this code under windows. I never 
> compiled a gdbserver on windows, is there any gdbserver hosted under 
> windows ?

If you were working against HEAD, you'd see that there was now - but
we don't support serial ports there, so it's not a problem.  It's all
#ifdef'd out.

-- 
Daniel Jacobowitz
CodeSourcery

2006-12-30  Denis PILAT <denis.pilat@st.com>
	    Daniel Jacobowitz  <dan@codesourcery.com>

	* remote-utils.c (remote_open): Check the type of specified
	serial port devices before opening them.
	* server.c (main): Kill the inferior if an error occurs during
	the first remote_open.

Index: remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.34
diff -u -p -r1.34 remote-utils.c
--- remote-utils.c	16 Nov 2006 15:08:25 -0000	1.34
+++ remote-utils.c	30 Dec 2006 15:23:37 -0000
@@ -52,6 +52,8 @@
 #if HAVE_ARPA_INET_H
 #include <arpa/inet.h>
 #endif
+#include <sys/stat.h>
+#include <errno.h>
 
 #if USE_WIN32API
 #include <winsock.h>
@@ -94,13 +96,25 @@ remote_open (char *name)
 #if defined(F_SETFL) && defined (FASYNC)
   int save_fcntl_flags;
 #endif
-  
-  if (!strchr (name, ':'))
+  char *port_str;
+
+  port_str = strchr (name, ':');
+  if (port_str == NULL)
     {
 #ifdef USE_WIN32API
       error ("Only <host>:<port> is supported on this platform.");
 #else
-      remote_desc = open (name, O_RDWR);
+      struct stat statbuf;
+
+      if (stat (name, &statbuf) == 0
+	  && (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode)))
+	remote_desc = open (name, O_RDWR);
+      else
+	{
+	  errno = EINVAL;
+	  remote_desc = -1;
+	}
+
       if (remote_desc < 0)
 	perror_with_name ("Could not open remote device");
 
Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.42
diff -u -p -r1.42 server.c
--- server.c	16 Nov 2006 15:08:25 -0000	1.42
+++ server.c	30 Dec 2006 15:23:37 -0000
@@ -614,6 +614,13 @@ main (int argc, char *argv[])
 	}
     }
 
+  if (setjmp (toplevel))
+    {
+      fprintf (stderr, "Killing inferior\n");
+      kill_inferior ();
+      exit (1);
+    }
+
   while (1)
     {
       remote_open (argv[1]);

Re: gdbserver with reversed arguments goes into an infinite loop

[Denis PILAT]

Daniel Jacobowitz wrote:
> On Wed, Dec 13, 2006 at 04:45:53PM +0100, Denis PILAT wrote:
>   
>> But may be it would be better to open only character device (S_ISCHR 
>> macro) than excluding ordinary files (S_ISREG macro). It's up to you !
>>     
>
> Good idea.  FIFOs are OK too.  I've committed a nicer version of this.
>
>   
>> I took this opportunity to remove a warning on a strncpy() usage.
>>     
>
> Why did it warn?  I omitted this bit, because I don't see any reason
> (or any warning).
>
>   
>> I'm wondering about the compilation of this code under windows. I never 
>> compiled a gdbserver on windows, is there any gdbserver hosted under 
>> windows ?
>>     
>
> If you were working against HEAD, you'd see that there was now - but
> we don't support serial ports there, so it's not a problem.  It's all
> #ifdef'd out.
>
>   
Daniel,
Thanks for your commit, I was working on a 6.5 version, so far from the 
HEAD.
About the warning, gcc4.1.1 emit the following:
    remote-utils.c:653: warning: value computed is not used
I think it is because the returned value of strncpy is not used but it 
seems to be a gcc4.1 problem. Moreover it occures only in -01 or -02.

About your patch, the infinite loop has gone but I'm having a 
segmentation fault when argument are reversed.
I'm about to propose a patch in a new mail to avoid confusion.

Denis