[PATCH 0/2] Fix problems with /proc/<pid>/fd checking

[PATCH 0/2] Fix problems with /proc/<pid>/fd checking

[Ken Brown]

This fixes the assertion failure reported here:

  https://sourceware.org/pipermail/cygwin/2020-September/246160.html

with a simple test case here:

  https://sourceware.org/pipermail/cygwin/2020-September/246188.html

That test case now fails as follows, as on Linux:

$ ./proc_bug.exe
open: Not a directory

I'm not completely sure about the second patch.  The path_conv::check
code is so complicated that I could easily have missed something.  But
I think it's correct.

Ken Brown (2):
  Cygwin: format_process_fd: add directory check
  Cygwin: path_conv::check: handle error from fhandler_process::exists

 winsup/cygwin/fhandler_process.cc | 15 +++++++++++++++
 winsup/cygwin/path.cc             |  6 ++++++
 2 files changed, 21 insertions(+)

-- 
2.28.0

[PATCH 1/2] Cygwin: format_process_fd: add directory check

[Ken Brown]

The incoming path is allowed to have the form "$PID/fd/[0-9]*/.*"
provided the descriptor symlink points to a directory.  Check that
this is indeed the case.
---
 winsup/cygwin/fhandler_process.cc | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/winsup/cygwin/fhandler_process.cc b/winsup/cygwin/fhandler_process.cc
index a6c358217..888604e3d 100644
--- a/winsup/cygwin/fhandler_process.cc
+++ b/winsup/cygwin/fhandler_process.cc
@@ -398,6 +398,21 @@ format_process_fd (void *data, char *&destbuf)
 	*((process_fd_t *) data)->fd_type = virt_fdsymlink;
       else /* trailing path */
 	{
+	  /* Does the descriptor link point to a directory? */
+	  bool dir;
+	  if (*destbuf != '/')	/* e.g., "pipe:[" or "socket:[" */
+	    dir = false;
+	  else
+	    {
+	      path_conv pc (destbuf);
+	      dir = pc.isdir ();
+	    }
+	  if (!dir)
+	    {
+	      set_errno (ENOTDIR);
+	      cfree (destbuf);
+	      return -1;
+	    }
 	  char *newbuf = (char *) cmalloc_abort (HEAP_STR, strlen (destbuf)
 							   + strlen (e) + 1);
 	  stpcpy (stpcpy (newbuf, destbuf), e);
-- 
2.28.0

[PATCH 2/2] Cygwin: path_conv::check: handle error from fhandler_process::exists

[Ken Brown]

fhandler_process::exists is called when we are checking a path
starting with "/proc/<pid>/fd".  If it returns virt_none and sets an
errno, there is no need for further checking.  Just set 'error' and
return.
---
 winsup/cygwin/path.cc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 95faf8ca7..092261939 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -809,6 +809,12 @@ path_conv::check (const char *src, unsigned opt,
 			  delete fh;
 			  goto retry_fs_via_processfd;
 			}
+		      else if (file_type == virt_none && dev == FH_PROCESSFD)
+			{
+			  error = get_errno ();
+			  if (error)
+			    return;
+			}
 		      delete fh;
 		    }
 		  switch (file_type)
-- 
2.28.0

Re: [PATCH 2/2] Cygwin: path_conv::check: handle error from fhandler_process::exists

[Ken Brown]

On 9/8/2020 12:50 PM, Ken Brown via Cygwin-patches wrote:
> fhandler_process::exists is called when we are checking a path
> starting with "/proc/<pid>/fd".  If it returns virt_none and sets an
> errno, there is no need for further checking.  Just set 'error' and
> return.
> ---
>   winsup/cygwin/path.cc | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
> index 95faf8ca7..092261939 100644
> --- a/winsup/cygwin/path.cc
> +++ b/winsup/cygwin/path.cc
> @@ -809,6 +809,12 @@ path_conv::check (const char *src, unsigned opt,
>   			  delete fh;
>   			  goto retry_fs_via_processfd;
>   			}
> +		      else if (file_type == virt_none && dev == FH_PROCESSFD)
> +			{
> +			  error = get_errno ();
> +			  if (error)
> +			    return;
> +			}
>   		      delete fh;
>   		    }
>   		  switch (file_type)
> 

There's a missing 'delete fh' above.  I'll send v2 shortly.

Ken

Re: [PATCH 1/2] Cygwin: format_process_fd: add directory check

[Corinna Vinschen]

Hi Ken,

On Sep  8 12:50, Ken Brown via Cygwin-patches wrote:
> The incoming path is allowed to have the form "$PID/fd/[0-9]*/.*"
> provided the descriptor symlink points to a directory.  Check that
> this is indeed the case.
> ---
>  winsup/cygwin/fhandler_process.cc | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/winsup/cygwin/fhandler_process.cc b/winsup/cygwin/fhandler_process.cc
> index a6c358217..888604e3d 100644
> --- a/winsup/cygwin/fhandler_process.cc
> +++ b/winsup/cygwin/fhandler_process.cc
> @@ -398,6 +398,21 @@ format_process_fd (void *data, char *&destbuf)
>  	*((process_fd_t *) data)->fd_type = virt_fdsymlink;
>        else /* trailing path */
>  	{
> +	  /* Does the descriptor link point to a directory? */
> +	  bool dir;
> +	  if (*destbuf != '/')	/* e.g., "pipe:[" or "socket:[" */
> +	    dir = false;
> +	  else
> +	    {
> +	      path_conv pc (destbuf);
> +	      dir = pc.isdir ();
> +	    }
> +	  if (!dir)
> +	    {
> +	      set_errno (ENOTDIR);
> +	      cfree (destbuf);
> +	      return -1;
> +	    }
>  	  char *newbuf = (char *) cmalloc_abort (HEAP_STR, strlen (destbuf)
>  							   + strlen (e) + 1);
>  	  stpcpy (stpcpy (newbuf, destbuf), e);
> -- 
> 2.28.0

Huh, I'd never realized this check is missing.  I was just puzzeling
over your patch, searching for the directory check, but yeah, there is
none.  Please push.


Thanks,
Corinna