Re: Add system(NULL) to fileio

[Corinna Vinschen <vinschen@redhat.com>]

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