[PATCH] Fix PR 19089: Environment variable TMP may yield gcc: abort

[PATCH] Fix PR 19089: Environment variable TMP may yield gcc: abort

[apinski]

From: Andrew Pinski <apinski@marvell.com>

Even though I cannot reproduce the ICE any more, this is still
a bug. We check already to see if we can access the directory
but never check to see if the path is actually a directory.

This adds the check and now we reject the file as not usable
as a tmp directory.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

libiberty/ChangeLog:

	* make-temp-file.c (try_dir): Check to see if the dir
	is actually a directory.
---
 libiberty/make-temp-file.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/libiberty/make-temp-file.c b/libiberty/make-temp-file.c
index 31f87fbcfde..11eb03d12ec 100644
--- a/libiberty/make-temp-file.c
+++ b/libiberty/make-temp-file.c
@@ -39,6 +39,10 @@ Boston, MA 02110-1301, USA.  */
 #if defined(_WIN32) && !defined(__CYGWIN__)
 #include <windows.h>
 #endif
+#if HAVE_SYS_STAT_H
+#include <sys/stat.h>
+#endif
+
 
 #ifndef R_OK
 #define R_OK 4
@@ -76,7 +80,17 @@ try_dir (const char *dir, const char *base)
     return base;
   if (dir != 0
       && access (dir, R_OK | W_OK | X_OK) == 0)
-    return dir;
+    {
+      /* Check to make sure dir is actually a directory. */
+#ifdef S_ISDIR
+      struct stat s;
+      if (stat(dir, &s))
+	return NULL;
+      if (!S_ISDIR (s.st_mode))
+	return NULL;
+#endif
+      return dir;
+    }
   return 0;
 }
 
-- 
2.27.0

Re: [PATCH] Fix PR 19089: Environment variable TMP may yield gcc: abort

[Jeff Law]

On 11/27/2021 7:49 PM, apinski--- via Gcc-patches wrote:
> From: Andrew Pinski <apinski@marvell.com>
>
> Even though I cannot reproduce the ICE any more, this is still
> a bug. We check already to see if we can access the directory
> but never check to see if the path is actually a directory.
>
> This adds the check and now we reject the file as not usable
> as a tmp directory.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> libiberty/ChangeLog:
>
> 	* make-temp-file.c (try_dir): Check to see if the dir
> 	is actually a directory.
> ---
>   libiberty/make-temp-file.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/libiberty/make-temp-file.c b/libiberty/make-temp-file.c
> index 31f87fbcfde..11eb03d12ec 100644
> --- a/libiberty/make-temp-file.c
> +++ b/libiberty/make-temp-file.c
> @@ -39,6 +39,10 @@ Boston, MA 02110-1301, USA.  */
>   #if defined(_WIN32) && !defined(__CYGWIN__)
>   #include <windows.h>
>   #endif
> +#if HAVE_SYS_STAT_H
> +#include <sys/stat.h>
> +#endif
> +
>   
>   #ifndef R_OK
>   #define R_OK 4
> @@ -76,7 +80,17 @@ try_dir (const char *dir, const char *base)
>       return base;
>     if (dir != 0
>         && access (dir, R_OK | W_OK | X_OK) == 0)
> -    return dir;
> +    {
> +      /* Check to make sure dir is actually a directory. */
> +#ifdef S_ISDIR
> +      struct stat s;
> +      if (stat(dir, &s))
Formatting nit, missing whitespace between stat and open paren.

Presumably this doesn't fix the problem in the case where S_ISDIR is not 
defined.  But it's still an improvement.  OK with the nit fixed.

jeff

Re: [PATCH] Fix PR 19089: Environment variable TMP may yield gcc: abort

[Andrew Pinski]

On Sun, Nov 28, 2021 at 12:14 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 11/27/2021 7:49 PM, apinski--- via Gcc-patches wrote:
> > From: Andrew Pinski <apinski@marvell.com>
> >
> > Even though I cannot reproduce the ICE any more, this is still
> > a bug. We check already to see if we can access the directory
> > but never check to see if the path is actually a directory.
> >
> > This adds the check and now we reject the file as not usable
> > as a tmp directory.
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> > libiberty/ChangeLog:
> >
> >       * make-temp-file.c (try_dir): Check to see if the dir
> >       is actually a directory.
> > ---
> >   libiberty/make-temp-file.c | 16 +++++++++++++++-
> >   1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/libiberty/make-temp-file.c b/libiberty/make-temp-file.c
> > index 31f87fbcfde..11eb03d12ec 100644
> > --- a/libiberty/make-temp-file.c
> > +++ b/libiberty/make-temp-file.c
> > @@ -39,6 +39,10 @@ Boston, MA 02110-1301, USA.  */
> >   #if defined(_WIN32) && !defined(__CYGWIN__)
> >   #include <windows.h>
> >   #endif
> > +#if HAVE_SYS_STAT_H
> > +#include <sys/stat.h>
> > +#endif
> > +
> >
> >   #ifndef R_OK
> >   #define R_OK 4
> > @@ -76,7 +80,17 @@ try_dir (const char *dir, const char *base)
> >       return base;
> >     if (dir != 0
> >         && access (dir, R_OK | W_OK | X_OK) == 0)
> > -    return dir;
> > +    {
> > +      /* Check to make sure dir is actually a directory. */
> > +#ifdef S_ISDIR
> > +      struct stat s;
> > +      if (stat(dir, &s))
> Formatting nit, missing whitespace between stat and open paren.
>
> Presumably this doesn't fix the problem in the case where S_ISDIR is not
> defined.  But it's still an improvement.  OK with the nit fixed.

Correct, though I don't know of any host where S_ISDIR is not defined.
Mingw has them defined. So does cygwin. glibc (and all libc on Linux)
has them defined, Solaris and AIX has them defined. So Does Mac OS X.


MSVC does not define them but we don't support MSVC to compile GCC so
that should not be an issue.

Thanks,
Andrew

>
> jeff
>

Re: [PATCH] Fix PR 19089: Environment variable TMP may yield gcc: abort

[Richard Biener]

On Mon, Nov 29, 2021 at 1:42 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Sun, Nov 28, 2021 at 12:14 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> >
> >
> > On 11/27/2021 7:49 PM, apinski--- via Gcc-patches wrote:
> > > From: Andrew Pinski <apinski@marvell.com>
> > >
> > > Even though I cannot reproduce the ICE any more, this is still
> > > a bug. We check already to see if we can access the directory
> > > but never check to see if the path is actually a directory.
> > >
> > > This adds the check and now we reject the file as not usable
> > > as a tmp directory.
> > >
> > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> > >
> > > libiberty/ChangeLog:
> > >
> > >       * make-temp-file.c (try_dir): Check to see if the dir
> > >       is actually a directory.
> > > ---
> > >   libiberty/make-temp-file.c | 16 +++++++++++++++-
> > >   1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libiberty/make-temp-file.c b/libiberty/make-temp-file.c
> > > index 31f87fbcfde..11eb03d12ec 100644
> > > --- a/libiberty/make-temp-file.c
> > > +++ b/libiberty/make-temp-file.c
> > > @@ -39,6 +39,10 @@ Boston, MA 02110-1301, USA.  */
> > >   #if defined(_WIN32) && !defined(__CYGWIN__)
> > >   #include <windows.h>
> > >   #endif
> > > +#if HAVE_SYS_STAT_H
> > > +#include <sys/stat.h>
> > > +#endif
> > > +
> > >
> > >   #ifndef R_OK
> > >   #define R_OK 4
> > > @@ -76,7 +80,17 @@ try_dir (const char *dir, const char *base)
> > >       return base;
> > >     if (dir != 0
> > >         && access (dir, R_OK | W_OK | X_OK) == 0)
> > > -    return dir;
> > > +    {
> > > +      /* Check to make sure dir is actually a directory. */
> > > +#ifdef S_ISDIR
> > > +      struct stat s;
> > > +      if (stat(dir, &s))

I wonder if we can instead do access (dir "/.") or so where access
should complain with ENOTDIR since 'dir' isn't a directory?

Richard.

> > Formatting nit, missing whitespace between stat and open paren.
> >
> > Presumably this doesn't fix the problem in the case where S_ISDIR is not
> > defined.  But it's still an improvement.  OK with the nit fixed.
>
> Correct, though I don't know of any host where S_ISDIR is not defined.
> Mingw has them defined. So does cygwin. glibc (and all libc on Linux)
> has them defined, Solaris and AIX has them defined. So Does Mac OS X.
>
>
> MSVC does not define them but we don't support MSVC to compile GCC so
> that should not be an issue.
>
> Thanks,
> Andrew
>
> >
> > jeff
> >

Re: [PATCH] Fix PR 19089: Environment variable TMP may yield gcc: abort

[Richard Biener]

On Mon, Nov 29, 2021 at 9:53 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Nov 29, 2021 at 1:42 AM Andrew Pinski via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Sun, Nov 28, 2021 at 12:14 PM Jeff Law via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > >
> > >
> > > On 11/27/2021 7:49 PM, apinski--- via Gcc-patches wrote:
> > > > From: Andrew Pinski <apinski@marvell.com>
> > > >
> > > > Even though I cannot reproduce the ICE any more, this is still
> > > > a bug. We check already to see if we can access the directory
> > > > but never check to see if the path is actually a directory.
> > > >
> > > > This adds the check and now we reject the file as not usable
> > > > as a tmp directory.
> > > >
> > > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> > > >
> > > > libiberty/ChangeLog:
> > > >
> > > >       * make-temp-file.c (try_dir): Check to see if the dir
> > > >       is actually a directory.
> > > > ---
> > > >   libiberty/make-temp-file.c | 16 +++++++++++++++-
> > > >   1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libiberty/make-temp-file.c b/libiberty/make-temp-file.c
> > > > index 31f87fbcfde..11eb03d12ec 100644
> > > > --- a/libiberty/make-temp-file.c
> > > > +++ b/libiberty/make-temp-file.c
> > > > @@ -39,6 +39,10 @@ Boston, MA 02110-1301, USA.  */
> > > >   #if defined(_WIN32) && !defined(__CYGWIN__)
> > > >   #include <windows.h>
> > > >   #endif
> > > > +#if HAVE_SYS_STAT_H
> > > > +#include <sys/stat.h>
> > > > +#endif
> > > > +
> > > >
> > > >   #ifndef R_OK
> > > >   #define R_OK 4
> > > > @@ -76,7 +80,17 @@ try_dir (const char *dir, const char *base)
> > > >       return base;
> > > >     if (dir != 0
> > > >         && access (dir, R_OK | W_OK | X_OK) == 0)
> > > > -    return dir;
> > > > +    {
> > > > +      /* Check to make sure dir is actually a directory. */
> > > > +#ifdef S_ISDIR
> > > > +      struct stat s;
> > > > +      if (stat(dir, &s))
>
> I wonder if we can instead do access (dir "/.") or so where access
> should complain with ENOTDIR since 'dir' isn't a directory?

On Linux it is enough to add DIR_SEPARATOR to 'dir' passed to access.
One would have to check other OSes (if they maybe fail to correctly
handle trailing DIR_SEPARATOR at all).

>
> Richard.
>
> > > Formatting nit, missing whitespace between stat and open paren.
> > >
> > > Presumably this doesn't fix the problem in the case where S_ISDIR is not
> > > defined.  But it's still an improvement.  OK with the nit fixed.
> >
> > Correct, though I don't know of any host where S_ISDIR is not defined.
> > Mingw has them defined. So does cygwin. glibc (and all libc on Linux)
> > has them defined, Solaris and AIX has them defined. So Does Mac OS X.
> >
> >
> > MSVC does not define them but we don't support MSVC to compile GCC so
> > that should not be an issue.
> >
> > Thanks,
> > Andrew
> >
> > >
> > > jeff
> > >