[PATCH] Cygwin: exec: check execute bit prior to evaluating script

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

[PATCH] Cygwin: exec: check execute bit prior to evaluating script

Corinna Vinschen-6
From: Corinna Vinschen <[hidden email]>

When the exec family of functions is called for a script-like
file, the av::setup function handles the exec[vl]p case as
well.  The execve case for files not starting with a she-bang
is handled first by returning ENOEXEC.  Only after that, the
file's executability is checked.

This leads to the problem that ENOEXEC is returned for non-executable
files as well.  A calling shell interprets this as a file it should try
to run as script.  This is not desired for non-executable files.

Fix this problem by checking the file for executability first.  Only
after that, follow the other potential code paths.
---
 winsup/cygwin/spawn.cc | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 7f7af4449da1..d95772802f8f 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -1172,6 +1172,12 @@ av::setup (const char *prog_arg, path_conv& real_path, const char *ext,
   }
  UnmapViewOfFile (buf);
   just_shell:
+ /* Check if script is executable.  Otherwise we start non-executable
+   scripts successfully, which is incorrect behaviour. */
+ if (real_path.has_acls ()
+    && check_file_access (real_path, X_OK, true) < 0)
+  return -1; /* errno is already set. */
+
  if (!pgm)
   {
     if (!p_type_exec)
@@ -1188,12 +1194,6 @@ av::setup (const char *prog_arg, path_conv& real_path, const char *ext,
     arg1 = NULL;
   }
 
- /* Check if script is executable.  Otherwise we start non-executable
-   scripts successfully, which is incorrect behaviour. */
- if (real_path.has_acls ()
-    && check_file_access (real_path, X_OK, true) < 0)
-  return -1; /* errno is already set. */
-
  /* Replace argv[0] with the full path to the script if this is the
    first time through the loop. */
  replace0_maybe (prog_arg);
--
2.20.1

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: exec: check execute bit prior to evaluating script

Ken Brown-6
On 8/6/2019 4:53 AM, Corinna Vinschen wrote:

> From: Corinna Vinschen <[hidden email]>
>
> When the exec family of functions is called for a script-like
> file, the av::setup function handles the exec[vl]p case as
> well.  The execve case for files not starting with a she-bang
> is handled first by returning ENOEXEC.  Only after that, the
> file's executability is checked.
>
> This leads to the problem that ENOEXEC is returned for non-executable
> files as well.  A calling shell interprets this as a file it should try
> to run as script.  This is not desired for non-executable files.
>
> Fix this problem by checking the file for executability first.  Only
> after that, follow the other potential code paths.
> ---
>   winsup/cygwin/spawn.cc | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> index 7f7af4449da1..d95772802f8f 100644
> --- a/winsup/cygwin/spawn.cc
> +++ b/winsup/cygwin/spawn.cc
> @@ -1172,6 +1172,12 @@ av::setup (const char *prog_arg, path_conv& real_path, const char *ext,
>    }
>   UnmapViewOfFile (buf);
>     just_shell:
> + /* Check if script is executable.  Otherwise we start non-executable
> +   scripts successfully, which is incorrect behaviour. */
> + if (real_path.has_acls ()
> +    && check_file_access (real_path, X_OK, true) < 0)
> +  return -1; /* errno is already set. */
> +
>   if (!pgm)
>    {
>      if (!p_type_exec)
> @@ -1188,12 +1194,6 @@ av::setup (const char *prog_arg, path_conv& real_path, const char *ext,
>      arg1 = NULL;
>    }
>  
> - /* Check if script is executable.  Otherwise we start non-executable
> -   scripts successfully, which is incorrect behaviour. */
> - if (real_path.has_acls ()
> -    && check_file_access (real_path, X_OK, true) < 0)
> -  return -1; /* errno is already set. */
> -
>   /* Replace argv[0] with the full path to the script if this is the
>     first time through the loop. */
>   replace0_maybe (prog_arg);

LGTM, and I've confirmed that it fixes the problem reported in
http://www.cygwin.org/ml/cygwin/2019-08/msg00054.html.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: exec: check execute bit prior to evaluating script

Corinna Vinschen-2
On Aug  6 12:09, Ken Brown wrote:

> On 8/6/2019 4:53 AM, Corinna Vinschen wrote:
> > From: Corinna Vinschen <[hidden email]>
> >
> > When the exec family of functions is called for a script-like
> > file, the av::setup function handles the exec[vl]p case as
> > well.  The execve case for files not starting with a she-bang
> > is handled first by returning ENOEXEC.  Only after that, the
> > file's executability is checked.
> >
> > This leads to the problem that ENOEXEC is returned for non-executable
> > files as well.  A calling shell interprets this as a file it should try
> > to run as script.  This is not desired for non-executable files.
> >
> > Fix this problem by checking the file for executability first.  Only
> > after that, follow the other potential code paths.
> > ---
> >   winsup/cygwin/spawn.cc | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> > index 7f7af4449da1..d95772802f8f 100644
> > --- a/winsup/cygwin/spawn.cc
> > +++ b/winsup/cygwin/spawn.cc
> > @@ -1172,6 +1172,12 @@ av::setup (const char *prog_arg, path_conv& real_path, const char *ext,
> >    }
> >   UnmapViewOfFile (buf);
> >     just_shell:
> > + /* Check if script is executable.  Otherwise we start non-executable
> > +   scripts successfully, which is incorrect behaviour. */
> > + if (real_path.has_acls ()
> > +    && check_file_access (real_path, X_OK, true) < 0)
> > +  return -1; /* errno is already set. */
> > +
> >   if (!pgm)
> >    {
> >      if (!p_type_exec)
> > @@ -1188,12 +1194,6 @@ av::setup (const char *prog_arg, path_conv& real_path, const char *ext,
> >      arg1 = NULL;
> >    }
> >  
> > - /* Check if script is executable.  Otherwise we start non-executable
> > -   scripts successfully, which is incorrect behaviour. */
> > - if (real_path.has_acls ()
> > -    && check_file_access (real_path, X_OK, true) < 0)
> > -  return -1; /* errno is already set. */
> > -
> >   /* Replace argv[0] with the full path to the script if this is the
> >     first time through the loop. */
> >   replace0_maybe (prog_arg);
>
> LGTM, and I've confirmed that it fixes the problem reported in
> http://www.cygwin.org/ml/cygwin/2019-08/msg00054.html.
>
> Ken
Thanks!  Pushed.


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment