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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

cygwin-patches mailing list
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

Reply | Threaded
Open this post in threaded view
|

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

cygwin-patches mailing list
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

Reply | Threaded
Open this post in threaded view
|

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

cygwin-patches mailing list
In reply to this post by cygwin-patches mailing list
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

Reply | Threaded
Open this post in threaded view
|

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

cygwin-patches mailing list
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
Reply | Threaded
Open this post in threaded view
|

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

Corinna Vinschen-2
In reply to this post by cygwin-patches mailing list
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