Add system(NULL) to fileio

Add system(NULL) to fileio

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

We noticed that system(NULL) has a special meaning.  This patch augments the
fileio protocol to support it.

Fortunately the current protocol's LENGTH field is strlen + 1, so it can never
legitimately be zero.  So I use that to indicate a NULL string is being passed
-- we don't have to presume a NULL pointer is all bits zero  :)

Tested with a modified libgloss for an m68k target. ok?

btw, this patch requires my testsuite patch for break on } to be applied.

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: sys-null.patch --]
[-- Type: text/x-patch, Size: 5701 bytes --]

2006-06-09  Nathan Sidwell  <nathan@codesourcery.com>

	gdb/
	* remote-file.io.c (remote_fileio_func_system): Treat zero length
	string as NULL.  Adjust for NULL pointer argument.
	* doc/gdb.texinfo (system): Document behaviour with zero length
	string.

	gdb/testsuite/
	* gdb.base/fileio.c: Add system(NULL) test.
	* gdb.base/fileio.exp: Check it.

Index: gdb/remote-fileio.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-fileio.c,v
retrieving revision 1.17.2.1
diff -c -3 -p -r1.17.2.1 remote-fileio.c
*** gdb/remote-fileio.c	24 May 2006 08:00:02 -0000	1.17.2.1
--- gdb/remote-fileio.c	9 Jun 2006 15:09:47 -0000
*************** remote_fileio_func_system (char *buf)
*** 1278,1293 ****
  {
    CORE_ADDR ptrval;
    int ret, length, retlength;
!   char *cmdline;
! 
!   /* Check if system(3) has been explicitely allowed using the
!      `set remote system-call-allowed 1' command.  If not, return
!      EPERM */
!   if (!remote_fio_system_call_allowed)
!     {
!       remote_fileio_reply (-1, FILEIO_EPERM);
!       return;
!     }
  
    /* Parameter: Ptr to commandline / length incl. trailing zero */
    if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length))
--- 1278,1284 ----
  {
    CORE_ADDR ptrval;
    int ret, length, retlength;
!   char *cmdline = NULL;
  
    /* Parameter: Ptr to commandline / length incl. trailing zero */
    if (remote_fileio_extract_ptr_w_len (&buf, &ptrval, &length))
*************** remote_fileio_func_system (char *buf)
*** 1295,1313 ****
        remote_fileio_ioerror ();
        return;
      }
!   /* Request commandline using 'm' packet */
!   cmdline = alloca (length);
!   retlength = remote_read_bytes (ptrval, (gdb_byte *) cmdline, length);
!   if (retlength != length)
      {
!       remote_fileio_ioerror ();
        return;
      }
  
    remote_fio_no_longjmp = 1;
    ret = system (cmdline);
  
!   if (ret == -1)
      remote_fileio_return_errno (-1);
    else
      remote_fileio_return_success (WEXITSTATUS (ret));
--- 1286,1322 ----
        remote_fileio_ioerror ();
        return;
      }
! 
!   if (length)
!     {
!       /* Request commandline using 'm' packet */
!       cmdline = alloca (length);
!       retlength = remote_read_bytes (ptrval, (gdb_byte *) cmdline, length);
!       if (retlength != length)
! 	{
! 	  remote_fileio_ioerror ();
! 	  return;
! 	}
!     }
!   
!   /* Check if system(3) has been explicitely allowed using the
!      `set remote system-call-allowed 1' command.  If not, return
!      EPERM */
!   if (!remote_fio_system_call_allowed)
      {
!       if (!length)
! 	remote_fileio_return_success (0);
!       else
! 	remote_fileio_reply (-1, FILEIO_EPERM);
        return;
      }
  
    remote_fio_no_longjmp = 1;
    ret = system (cmdline);
  
!   if (!length)
!     remote_fileio_return_success (ret);
!   else if (ret == -1)
      remote_fileio_return_errno (-1);
    else
      remote_fileio_return_success (WEXITSTATUS (ret));
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.314.2.2
diff -c -3 -p -r1.314.2.2 gdb.texinfo
*** gdb/doc/gdb.texinfo	19 Apr 2006 18:19:20 -0000	1.314.2.2
--- gdb/doc/gdb.texinfo	9 Jun 2006 15:09:57 -0000
*************** int system(const char *command);
*** 24564,24574 ****
  Fsystem,commandptr/len
  
  @exdent Return value:
! The value returned is -1 on error and the return status
! of the command otherwise.  Only the exit status of the
! command is returned, which is extracted from the hosts
! system return value by calling WEXITSTATUS(retval).
! In case /bin/sh could not be executed, 127 is returned.
  
  @exdent Errors:
  @end smallexample
--- 24564,24576 ----
  Fsystem,commandptr/len
  
  @exdent Return value:
! If @var{len} is zero, the return value indicates whether a shell is
! available.  Zero indicates it is not available and non-zero indicates
! that it is.  Otherwise, the value returned is -1 on error and the
! return status of the command otherwise.  Only the exit status of the
! command is returned, which is extracted from the hosts system return
! value by calling WEXITSTATUS(retval).  In case /bin/sh could not be
! executed, 127 is returned.
  
  @exdent Errors:
  @end smallexample
Index: gdb/testsuite/gdb.base/fileio.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/fileio.c,v
retrieving revision 1.8.12.1
diff -c -3 -p -r1.8.12.1 fileio.c
*** gdb/testsuite/gdb.base/fileio.c	5 Jun 2006 15:36:02 -0000	1.8.12.1
--- gdb/testsuite/gdb.base/fileio.c	9 Jun 2006 15:12:57 -0000
*************** test_system ()
*** 385,390 ****
--- 385,394 ----
    ret = system ("wrtzlpfrmpft");
    printf ("system 2: ret = %d %s\n", ret, WEXITSTATUS (ret) == 127 ? "OK" : "");
    stop ();
+   /* Test for shell */
+   ret = system (NULL);
+   printf ("system 3: ret = %d %s\n", ret, ret != 0 ? "OK" : "");
+   stop ();
  }
  
  int
Index: gdb/testsuite/gdb.base/fileio.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/fileio.exp,v
retrieving revision 1.7.30.1
diff -c -3 -p -r1.7.30.1 fileio.exp
*** gdb/testsuite/gdb.base/fileio.exp	5 Jun 2006 15:39:14 -0000	1.7.30.1
--- gdb/testsuite/gdb.base/fileio.exp	9 Jun 2006 15:12:57 -0000
*************** gdb_test continue \
*** 191,196 ****
--- 191,200 ----
  "System with invalid command returns 127"
  
  gdb_test continue \
+ "Continuing\\..*system 3:.*OK$stop_msg" \
+ "System says shell is available"
+ 
+ gdb_test continue \
  "Continuing\\..*rename 1:.*OK$stop_msg" \
  "Rename a file"
  

Re: Add system(NULL) to fileio

[Eli Zaretskii]

> Date: Fri, 09 Jun 2006 21:20:52 +0100
> From: Nathan Sidwell <nathan@codesourcery.com>
> 
> Tested with a modified libgloss for an m68k target. ok?

Thanks.

The patch to gdb.texinfo is approved, conditioned on the approval of
the code patch, provided that you take care of the comment below:

> ! If @var{len} is zero, the return value indicates whether a shell is
> ! available.  Zero indicates it is not available and non-zero indicates
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I suggest "A zero return value indicates a shell is not available..."
I think this makes the text less ambiguous.

> *** gdb/testsuite/gdb.base/fileio.c	5 Jun 2006 15:36:02 -0000	1.8.12.1
> --- gdb/testsuite/gdb.base/fileio.c	9 Jun 2006 15:12:57 -0000
> *************** test_system ()
> *** 385,390 ****
> --- 385,394 ----
>     ret = system ("wrtzlpfrmpft");
>     printf ("system 2: ret = %d %s\n", ret, WEXITSTATUS (ret) == 127 ? "OK" : "");
>     stop ();
> +   /* Test for shell */
> +   ret = system (NULL);
> +   printf ("system 3: ret = %d %s\n", ret, ret != 0 ? "OK" : "");
> +   stop ();

Isn't it better to test for shell availability _before_ we send it
commands, not after?

Re: Add system(NULL) to fileio

[Nathan Sidwell]

Eli Zaretskii wrote:

>> *** gdb/testsuite/gdb.base/fileio.c	5 Jun 2006 15:36:02 -0000	1.8.12.1
>> --- gdb/testsuite/gdb.base/fileio.c	9 Jun 2006 15:12:57 -0000
>> *************** test_system ()
>> *** 385,390 ****
>> --- 385,394 ----
>>     ret = system ("wrtzlpfrmpft");
>>     printf ("system 2: ret = %d %s\n", ret, WEXITSTATUS (ret) == 127 ? "OK" : "");
>>     stop ();
>> +   /* Test for shell */
>> +   ret = system (NULL);
>> +   printf ("system 3: ret = %d %s\n", ret, ret != 0 ? "OK" : "");
>> +   stop ();
> 
> Isn't it better to test for shell availability _before_ we send it
> commands, not after?

Sure.  I just didn't want to disturb the testcase more.  I can reorder it  :)

Can you look at my 'Remove some spurious test fails' because, amongst other
things it makes it tractable to go alter the fileio testcase.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: Add system(NULL) to fileio

[Eli Zaretskii]

> Date: Sat, 10 Jun 2006 12:51:40 +0100
> From: Nathan Sidwell <nathan@codesourcery.com>
> CC:  gdb-patches@sourceware.org
> 
> Can you look at my 'Remove some spurious test fails' because, amongst other
> things it makes it tractable to go alter the fileio testcase.

I believe Daniel will review that.

Re: Add system(NULL) to fileio

[Corinna Vinschen]

On Jun  9 21:20, Nathan Sidwell wrote:
> We noticed that system(NULL) has a special meaning.  This patch augments the
> fileio protocol to support it.
> 
> Fortunately the current protocol's LENGTH field is strlen + 1, so it can never
> legitimately be zero.  So I use that to indicate a NULL string is being passed
> -- we don't have to presume a NULL pointer is all bits zero  :)
> 
> Tested with a modified libgloss for an m68k target. ok?
> [...]

As for the actual code, since I wrote the original fileio stuff, I thought
I could have a look.

> !   if (length)
> !     {
> !       /* Request commandline using 'm' packet */
> !       cmdline = alloca (length);
> !       retlength = remote_read_bytes (ptrval, (gdb_byte *) cmdline, length);
> !       if (retlength != length)
> ! 	{
> ! 	  remote_fileio_ioerror ();
> ! 	  return;
> ! 	}
> !     }
> !   
> !   /* Check if system(3) has been explicitely allowed using the
> !      `set remote system-call-allowed 1' command.  If not, return
> !      EPERM */

Wouldn't it be better to add a short comment here explaining the !length
case, something along the lines of "A zero length indicates a NULL
argument to system(3) ... handled according to POSIX (check if shell
exists) even if the system call hasn't been allowed by the user"?

> !   if (!remote_fio_system_call_allowed)
>       {
> !       if (!length)
> ! 	remote_fileio_return_success (0);
> !       else
> ! 	remote_fileio_reply (-1, FILEIO_EPERM);
>         return;
>       }
>   
>     remote_fio_no_longjmp = 1;
>     ret = system (cmdline);
>   
> !   if (!length)
> !     remote_fileio_return_success (ret);
> !   else if (ret == -1)
>       remote_fileio_return_errno (-1);
>     else
>       remote_fileio_return_success (WEXITSTATUS (ret));

Everything else looks good to me.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: Add system(NULL) to fileio

[Nathan Sidwell]

Corinna Vinschen wrote:

> 
> Wouldn't it be better to add a short comment here explaining the !length
> case, something along the lines of "A zero length indicates a NULL
> argument to system(3) ... handled according to POSIX (check if shell
> exists) even if the system call hasn't been allowed by the user"?

I'll add such a comment, thanks for the review.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: Add system(NULL) to fileio

[Nathan Sidwell]

Nathan Sidwell wrote:

> ------------------------------------------------------------------------
> 
> 2006-06-09  Nathan Sidwell  <nathan@codesourcery.com>
> 
> 	gdb/
> 	* remote-file.io.c (remote_fileio_func_system): Treat zero length
> 	string as NULL.  Adjust for NULL pointer argument.
> 	* doc/gdb.texinfo (system): Document behaviour with zero length
> 	string.
> 
> 	gdb/testsuite/
> 	* gdb.base/fileio.c: Add system(NULL) test.
> 	* gdb.base/fileio.exp: Check it.

I applied the patch with the suggested changes to documentation and comment. 
Daniel pointed out that I'd misunderstood Eli's email as only approving the 
documentation change, but he has told me the patch is good.

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk