Re: [PATCH] gcc cannot deal with full /tmp

Re: [PATCH] gcc cannot deal with full /tmp

[Zack Weinberg]

+      if (errno != EEXIST)
+	/* Fatal error (EPERM, ENOSPC etc).  Doesn't make sense to loop.  */
+	break;

You should also loop on EISDIR.

zw

Re: [PATCH] gcc cannot deal with full /tmp

[Ian Lance Taylor]

"Zack Weinberg" <zackw@panix.com> writes:

> +      if (errno != EEXIST)
> +	/* Fatal error (EPERM, ENOSPC etc).  Doesn't make sense to loop.  */
> +	break;
>
> You should also loop on EISDIR.

Good point, thanks, Jakub that change is preapproved.

Ian

Re: [PATCH] gcc cannot deal with full /tmp

[Denys Vlasenko]

On Thursday 31 July 2008 11:04:28 pm Ian Lance Taylor wrote:
> "Zack Weinberg" <zackw@panix.com> writes:
> 
> > +      if (errno != EEXIST)
> > +	/* Fatal error (EPERM, ENOSPC etc).  Doesn't make sense to loop.  */
> > +	break;
> >
> > You should also loop on EISDIR.
> 
> Good point, thanks, Jakub that change is preapproved.

# cat t.c
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

int main()
{
        return open("/tmp", O_WRONLY | O_CREAT | O_EXCL);
}
# gcc t.c
# strace ./a.out
execve("./a.out", ["./a.out"], [/* 54 vars */]) = 0
brk(0)                                  = 0xd9c000
....
munmap(0x7f985dbfc000, 100172)          = 0
open("/tmp", O_WRONLY|O_CREAT|O_EXCL, 03777754560257210) = -1 EEXIST (File exists)
exit_group(-1)                          = ?

At least on my system it throws EEXIST even on dirs.
However, I agree with you that on other Unixes it can be different.

My original approach was to reduce number of loops
instead of bailing out _immediately_ on errno != EEXIST.
I feared that otherwise I'll have to deal with
"obscure system XYZ throws weird errno at me" syndrome.

This is it coming true.
--
vda

Re: [PATCH] gcc cannot deal with full /tmp

[Zack Weinberg]

On Fri, Aug 1, 2008 at 3:57 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
[...]
> My original approach was to reduce number of loops
> instead of bailing out _immediately_ on errno != EEXIST.
> I feared that otherwise I'll have to deal with
> "obscure system XYZ throws weird errno at me" syndrome.
>
> This is it coming true.

I'm quite confident that EEXIST and EISDIR are the only two errno
codes that should trigger looping.

zw

Re: [PATCH] gcc cannot deal with full /tmp

[Denys Vlasenko]

On Friday 01 August 2008 11:51:04 pm Zack Weinberg wrote:
> On Fri, Aug 1, 2008 at 3:57 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> [...]
> > My original approach was to reduce number of loops
> > instead of bailing out _immediately_ on errno != EEXIST.
> > I feared that otherwise I'll have to deal with
> > "obscure system XYZ throws weird errno at me" syndrome.
> >
> > This is it coming true.
> 
> I'm quite confident that EEXIST and EISDIR are the only two errno
> codes that should trigger looping.

I am confident that only EEXIST should trigger looping,
because I just tested that opening a dir with O_EXCL
returns EEXIST, not EISDIR.

I will promptly change my mind as soon as I will be bitten
by some other Unix throwing EISDIR on me.

Then, after a few years, when yet another Unix will also
bite me by throwing a ELOOP on an attempt of opening
a self-referential symlink in /tmp, I will add a check
for ELOOP too.

And so on. EFBIG, ETXTBSY, maybe even ENODEV are in store.

Or maybe I should forestall multiple iterations of fixing
of the same bug, don't you think?
--
vda

Re: [PATCH] gcc cannot deal with full /tmp

[Zack Weinberg]

On Mon, Aug 4, 2008 at 4:02 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> On Friday 01 August 2008 11:51:04 pm Zack Weinberg wrote:
>>
>> I'm quite confident that EEXIST and EISDIR are the only two errno
>> codes that should trigger looping.
>
> I am confident that only EEXIST should trigger looping,
> because I just tested that opening a dir with O_EXCL
> returns EEXIST, not EISDIR.
>
> I will promptly change my mind as soon as I will be bitten
> by some other Unix throwing EISDIR on me.
>
> Then, after a few years, when yet another Unix will also
> bite me by throwing a ELOOP on an attempt of opening
> a self-referential symlink in /tmp, I will add a check
> for ELOOP too.
>
> And so on. EFBIG, ETXTBSY, maybe even ENODEV are in store.

The way I come to the conclusion that you should loop on EEXIST,
EISDIR, and no others is, I assume that the open() call could produce
_any_ code in errno.h, and then I think about what they mean.  EEXIST
obviously describes a condition where you should loop.  So does
EISDIR, because you could plausibly get that code just because there's
a directory with the same name as your target file in /tmp.  (I'm
pretty sure I've used Unices that did that.)

But none of the others you suggest could come back under normal
operating conditions, even if you had an OS that was insisting on
telling you _what_ the conflicting name was by returning ELOOP for a
bad symlink, EFBIG for a giant file because you didn't call open64()
or whatever the official way to do that is, now -- because the only
things that normally appear loose in /tmp are small plain files and
directories. If you've got something exotic in /tmp with a name that
conflicts with a compiler scratch filename, you _should_ fail the
creation of the scratch file, because something weird is going on.

zw

Re: [PATCH] gcc cannot deal with full /tmp

[Denys Vlasenko]

On Sat, 2008-08-16 at 21:43 -0700, Zack Weinberg wrote:
> On Mon, Aug 4, 2008 at 4:02 AM, Denys Vlasenko <dvlasenk@redhat.com> wrote:
> > On Friday 01 August 2008 11:51:04 pm Zack Weinberg wrote:
> >>
> >> I'm quite confident that EEXIST and EISDIR are the only two errno
> >> codes that should trigger looping.
> >
> > I am confident that only EEXIST should trigger looping,
> > because I just tested that opening a dir with O_EXCL
> > returns EEXIST, not EISDIR.
> >
> > I will promptly change my mind as soon as I will be bitten
> > by some other Unix throwing EISDIR on me.
> >
> > Then, after a few years, when yet another Unix will also
> > bite me by throwing a ELOOP on an attempt of opening
> > a self-referential symlink in /tmp, I will add a check
> > for ELOOP too.
> >
> > And so on. EFBIG, ETXTBSY, maybe even ENODEV are in store.
> 
> The way I come to the conclusion that you should loop on EEXIST,
> EISDIR, and no others is, I assume that the open() call could produce
> _any_ code in errno.h, and then I think about what they mean.  EEXIST
> obviously describes a condition where you should loop.  So does
> EISDIR, because you could plausibly get that code just because there's
> a directory with the same name as your target file in /tmp.  (I'm
> pretty sure I've used Unices that did that.)
> 
> But none of the others you suggest could come back under normal
> operating conditions,

I disagree.

>  even if you had an OS that was insisting on
> telling you _what_ the conflicting name was by returning ELOOP for a
> bad symlink, EFBIG for a giant file because you didn't call open64()
> or whatever the official way to do that is, now -- because the only
> things that normally appear loose in /tmp are small plain files and
> directories. If you've got something exotic in /tmp with a name that
> conflicts with a compiler scratch filename, you _should_ fail the
> creation of the scratch file, because something weird is going on.

ELOOP is a good counter-example. "ln -s bogus /tmp/bogus" is not such
an implausible operation. Any user can do that.
--
vda

Re: [PATCH] gcc cannot deal with full /tmp

[Jakub Jelinek]

On Mon, Aug 18, 2008 at 01:30:48PM +0200, Denys Vlasenko wrote:
> On Sat, 2008-08-16 at 21:43 -0700, Zack Weinberg wrote:
> >  even if you had an OS that was insisting on
> > telling you _what_ the conflicting name was by returning ELOOP for a
> > bad symlink, EFBIG for a giant file because you didn't call open64()
> > or whatever the official way to do that is, now -- because the only
> > things that normally appear loose in /tmp are small plain files and
> > directories. If you've got something exotic in /tmp with a name that
> > conflicts with a compiler scratch filename, you _should_ fail the
> > creation of the scratch file, because something weird is going on.
> 
> ELOOP is a good counter-example. "ln -s bogus /tmp/bogus" is not such
> an implausible operation. Any user can do that.

Can you find a single OS which gives you ELOOP for O_EXCL creat?

#include <unistd.h>
#include <fcntl.h>

int
main (void)
{
  unlink ("/tmp/bogus");
  symlink ("bogus", "/tmp/bogus");
  int fd = open ("/tmp/bogus", O_CREAT | O_RDWR | O_EXCL, 0644);
  if (fd < 0)
    perror ("");
  return 0;
}

	Jakub

Re: [PATCH] gcc cannot deal with full /tmp

[Denys Vlasenko]

On Mon, 2008-08-18 at 13:44 +0200, Jakub Jelinek wrote:
> On Mon, Aug 18, 2008 at 01:30:48PM +0200, Denys Vlasenko wrote:
> > On Sat, 2008-08-16 at 21:43 -0700, Zack Weinberg wrote:
> > >  even if you had an OS that was insisting on
> > > telling you _what_ the conflicting name was by returning ELOOP for a
> > > bad symlink, EFBIG for a giant file because you didn't call open64()
> > > or whatever the official way to do that is, now -- because the only
> > > things that normally appear loose in /tmp are small plain files and
> > > directories. If you've got something exotic in /tmp with a name that
> > > conflicts with a compiler scratch filename, you _should_ fail the
> > > creation of the scratch file, because something weird is going on.
> > 
> > ELOOP is a good counter-example. "ln -s bogus /tmp/bogus" is not such
> > an implausible operation. Any user can do that.
> 
> Can you find a single OS which gives you ELOOP for O_EXCL creat?

No.

But I cannot find an OS which gives me EISDIR either,
thus the same argument works against EISDIR all the same.

My argument is that it's better to proactively assume that there can be
some OSes which do it, or will do it in the future, and code around it
so that we won't need to revisit this issue.
--
vda

Re: [PATCH] gcc cannot deal with full /tmp

[Andreas Schwab]

Denys Vlasenko <dvlasenk@redhat.com> writes:

> On Mon, 2008-08-18 at 13:44 +0200, Jakub Jelinek wrote:
>> On Mon, Aug 18, 2008 at 01:30:48PM +0200, Denys Vlasenko wrote:
>> > On Sat, 2008-08-16 at 21:43 -0700, Zack Weinberg wrote:
>> > >  even if you had an OS that was insisting on
>> > > telling you _what_ the conflicting name was by returning ELOOP for a
>> > > bad symlink, EFBIG for a giant file because you didn't call open64()
>> > > or whatever the official way to do that is, now -- because the only
>> > > things that normally appear loose in /tmp are small plain files and
>> > > directories. If you've got something exotic in /tmp with a name that
>> > > conflicts with a compiler scratch filename, you _should_ fail the
>> > > creation of the scratch file, because something weird is going on.
>> > 
>> > ELOOP is a good counter-example. "ln -s bogus /tmp/bogus" is not such
>> > an implausible operation. Any user can do that.
>> 
>> Can you find a single OS which gives you ELOOP for O_EXCL creat?
>
> No.
>
> But I cannot find an OS which gives me EISDIR either,
> thus the same argument works against EISDIR all the same.
>
> My argument is that it's better to proactively assume that there can be
> some OSes which do it, or will do it in the future, and code around it
> so that we won't need to revisit this issue.

If you can't create a file in /tmp for a reason other than `file exists'
trying a different name is unlikely to succeed either, so in practice,
EEXIST is the only interesting error.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] gcc cannot deal with full /tmp

[Denys Vlasenko]

On Mon, 2008-08-18 at 14:09 +0200, Andreas Schwab wrote:
> >> > ELOOP is a good counter-example. "ln -s bogus /tmp/bogus" is not such
> >> > an implausible operation. Any user can do that.
> >> 
> >> Can you find a single OS which gives you ELOOP for O_EXCL creat?
> >
> > No.
> >
> > But I cannot find an OS which gives me EISDIR either,
> > thus the same argument works against EISDIR all the same.
> >
> > My argument is that it's better to proactively assume that there can be
> > some OSes which do it, or will do it in the future, and code around it
> > so that we won't need to revisit this issue.
> 
> If you can't create a file in /tmp for a reason other than `file exists'
> trying a different name is unlikely to succeed either, so in practice,
> EEXIST is the only interesting error.

Did you read the whole thread? Specifically, the below mail?
It's not *me* who said that we should check other errno's.

Moreover, at https://bugzilla.redhat.com/show_bug.cgi?id=203231
I *did* attach a patch which checks only for EEXIST.


On Fri, 2008-08-01 at 14:51 -0700, Zack Weinberg wrote:
On Fri, Aug 1, 2008 at 3:57 AM, Denys Vlasenko <dvlasenk@redhat.com>
wrote:
> [...]
> > My original approach was to reduce number of loops
> > instead of bailing out _immediately_ on errno != EEXIST.
> > I feared that otherwise I'll have to deal with
> > "obscure system XYZ throws weird errno at me" syndrome.
> >
> > This is it coming true.
> 
> I'm quite confident that EEXIST and EISDIR are the only two errno
> codes that should trigger looping.


--
vda

Re: [PATCH] gcc cannot deal with full /tmp

[Andreas Schwab]

Denys Vlasenko <dvlasenk@redhat.com> writes:

> On Mon, 2008-08-18 at 14:09 +0200, Andreas Schwab wrote:
>> >> > ELOOP is a good counter-example. "ln -s bogus /tmp/bogus" is not such
>> >> > an implausible operation. Any user can do that.
>> >> 
>> >> Can you find a single OS which gives you ELOOP for O_EXCL creat?
>> >
>> > No.
>> >
>> > But I cannot find an OS which gives me EISDIR either,
>> > thus the same argument works against EISDIR all the same.
>> >
>> > My argument is that it's better to proactively assume that there can be
>> > some OSes which do it, or will do it in the future, and code around it
>> > so that we won't need to revisit this issue.
>> 
>> If you can't create a file in /tmp for a reason other than `file exists'
>> trying a different name is unlikely to succeed either, so in practice,
>> EEXIST is the only interesting error.
>
> Did you read the whole thread? Specifically, the below mail?
> It's not *me* who said that we should check other errno's.

I did not claim that.

> Moreover, at https://bugzilla.redhat.com/show_bug.cgi?id=203231
> I *did* attach a patch which checks only for EEXIST.

Which my statement completely agrees with.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] gcc cannot deal with full /tmp

[Ian Lance Taylor]

Jakub Jelinek <jakub@redhat.com> writes:

> 2008-07-31  Denys Vlasenko  <dvlasenk@redhat.com>
>
> 	* mkstemps.c (mkstemps): If open failed with errno other than
> 	EEXIST, return immediately.
> 	* make-temp-file.c: Include errno.h.
> 	(make_temp_file): If mkstemps failed, print an error message
> 	before aborting.

This is OK.

Thanks.

Ian

Re: [PATCH] gcc cannot deal with full /tmp

[Jakub Jelinek]

On Wed, Jul 30, 2008 at 07:02:11AM -0700, Ian Lance Taylor wrote:
> Denys Vlasenko <dvlasenk@redhat.com> writes:
> 
> >> Otherwise this seems good.  How did you test it?
> >
> > umount -d testdir 2>/dev/null
> > rm -rf testdir 2>/dev/null
> > mkdir testdir 2>/dev/null
> > dd if=/dev/zero of=image bs=1M count=1
> > chmod 666 image
> > mkfs.minix image || exit 1
> > mount -o loop image testdir
> > TMP=$PWD/testdir i486-linux-uclibc-gcc -o t t.c
> 
> gcc patches also need to be tested with a full bootstrap, which you
> can do by simply running "make".  Thanks.

I've bootstrapped/regtested it for Denys on x86_64-linux last night.
Here is an updated version with CL, fixed comment spacing and spaces between
fn name and (.

2008-07-31  Denys Vlasenko  <dvlasenk@redhat.com>

	* mkstemps.c (mkstemps): If open failed with errno other than
	EEXIST, return immediately.
	* make-temp-file.c: Include errno.h.
	(make_temp_file): If mkstemps failed, print an error message
	before aborting.

--- libiberty/mkstemps.c	2008-07-21 14:50:00.000000000 +0200
+++ libiberty/mkstemps.c	2008-07-28 18:48:34.000000000 +0200
@@ -127,6 +127,9 @@ mkstemps (char *pattern, int suffix_len)
       if (fd >= 0)
 	/* The file does not exist.  */
 	return fd;
+      if (errno != EEXIST)
+	/* Fatal error (EPERM, ENOSPC etc).  Doesn't make sense to loop.  */
+	break;
 
       /* This is a random value.  It is only necessary that the next
 	 TMP_MAX values generated by adding 7777 to VALUE are different
--- libiberty/make-temp-file.c	2008-07-21 14:50:00.000000000 +0200
+++ libiberty/make-temp-file.c	2008-07-30 13:23:04.000000000 +0200
@@ -23,6 +23,7 @@ Boston, MA 02110-1301, USA.  */
 
 #include <stdio.h>	/* May get P_tmpdir.  */
 #include <sys/types.h>
+#include <errno.h>
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>
 #endif
@@ -166,11 +167,14 @@ make_temp_file (const char *suffix)
   strcpy (temp_filename + base_len + TEMP_FILE_LEN, suffix);
 
   fd = mkstemps (temp_filename, suffix_len);
-  /* If mkstemps failed, then something bad is happening.  Maybe we should
-     issue a message about a possible security attack in progress?  */
+  /* Mkstemps failed.  It may be EPERM, ENOSPC etc.  */
   if (fd == -1)
-    abort ();
-  /* Similarly if we can not close the file.  */
+    {
+      fprintf (stderr, "Cannot create temporary file in %s: %s\n",
+	       base, strerror (errno));
+      abort ();
+    }
+  /* We abort on failed close out of sheer paranoia.  */
   if (close (fd))
     abort ();
   return temp_filename;

	Jakub

Re: [PATCH] gcc cannot deal with full /tmp

[Ian Lance Taylor]

Denys Vlasenko <dvlasenk@redhat.com> writes:

>> Otherwise this seems good.  How did you test it?
>
> umount -d testdir 2>/dev/null
> rm -rf testdir 2>/dev/null
> mkdir testdir 2>/dev/null
> dd if=/dev/zero of=image bs=1M count=1
> chmod 666 image
> mkfs.minix image || exit 1
> mount -o loop image testdir
> TMP=$PWD/testdir i486-linux-uclibc-gcc -o t t.c

gcc patches also need to be tested with a full bootstrap, which you
can do by simply running "make".  Thanks.

Ian

Re: [PATCH] gcc cannot deal with full /tmp

[Denys Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Tuesday 29 July 2008 04:23:21 pm Ian Lance Taylor wrote:
> make_temp_file is documented as returning NULL on failure.  Would it
> be reasonable to simply return NULL for certain errno codes?

I don't know. This can be done incrementally. For gcc,
aborting on unwritable/full /tmp is ok, I guess.

> As far as the patch goes:
> * Comments should be complete sentences.
> * The fprintf line is too long--it needs to wrap at 80 columns.
> * Don't put a comment on the same line as abort ().

Updated patch is attached, is it ok now?

> Otherwise this seems good.  How did you test it?

umount -d testdir 2>/dev/null
rm -rf testdir 2>/dev/null
mkdir testdir 2>/dev/null
dd if=/dev/zero of=image bs=1M count=1
chmod 666 image
mkfs.minix image || exit 1
mount -o loop image testdir
TMP=$PWD/testdir i486-linux-uclibc-gcc -o t t.c

For some strange reason on my machine image thus mounted
is not writable, so I didn't have to do anything special
to make open's fail.

If you won't be so lucky, I guess you will need to add
chmod 111 -Rc testdir
just below mount command, and run gcc as non-root.
--
vda

[-- Attachment #2: 1.patch --]
[-- Type: text/x-diff, Size: 1695 bytes --]

diff -d -urpN gcc.0/libiberty/make-temp-file.c gcc.1/libiberty/make-temp-file.c
--- gcc.0/libiberty/make-temp-file.c	2008-07-21 14:50:00.000000000 +0200
+++ gcc.1/libiberty/make-temp-file.c	2008-07-30 13:23:04.000000000 +0200
@@ -23,6 +23,7 @@ Boston, MA 02110-1301, USA.  */
 
 #include <stdio.h>	/* May get P_tmpdir.  */
 #include <sys/types.h>
+#include <errno.h>
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>
 #endif
@@ -166,11 +167,14 @@ make_temp_file (const char *suffix)
   strcpy (temp_filename + base_len + TEMP_FILE_LEN, suffix);
 
   fd = mkstemps (temp_filename, suffix_len);
-  /* If mkstemps failed, then something bad is happening.  Maybe we should
-     issue a message about a possible security attack in progress?  */
+  /* Mkstemps failed.  It may be EPERM, ENOSPC etc.  */
   if (fd == -1)
-    abort ();
-  /* Similarly if we can not close the file.  */
+    {
+      fprintf(stderr, "Cannot create temporary file in %s: %s\n",
+	      base, strerror(errno));
+      abort ();
+    }
+  /* We abort on failed close out of sheer paranoia.  */
   if (close (fd))
     abort ();
   return temp_filename;
diff -d -urpN gcc.0/libiberty/mkstemps.c gcc.1/libiberty/mkstemps.c
--- gcc.0/libiberty/mkstemps.c	2008-07-21 14:50:00.000000000 +0200
+++ gcc.1/libiberty/mkstemps.c	2008-07-28 18:48:34.000000000 +0200
@@ -127,6 +127,9 @@ mkstemps (char *pattern, int suffix_len)
       if (fd >= 0)
 	/* The file does not exist.  */
 	return fd;
+      if (errno != EEXIST)
+	/* Fatal error (EPERM, ENOSPC etc). Doesn't make sense to loop.  */
+	break;
 
       /* This is a random value.  It is only necessary that the next
 	 TMP_MAX values generated by adding 7777 to VALUE are different

Re: [PATCH] gcc cannot deal with full /tmp

[Jakub Jelinek]

On Tue, Jul 29, 2008 at 07:23:21AM -0700, Ian Lance Taylor wrote:
> As far as the patch goes:
> * Comments should be complete sentences.
> * The fprintf line is too long--it needs to wrap at 80 columns.
> * Don't put a comment on the same line as abort ().

Also:
* Put a space between function name and opening (.
* Two spaces between sentences in comments.
* Write a ChangeLog entry.

	Jakub

Re: [PATCH] gcc cannot deal with full /tmp

[Ian Lance Taylor]

Denys Vlasenko <dvlasenk@redhat.com> writes:

> @@ -166,12 +167,13 @@ make_temp_file (const char *suffix)
>    strcpy (temp_filename + base_len + TEMP_FILE_LEN, suffix);
>  
>    fd = mkstemps (temp_filename, suffix_len);
> -  /* If mkstemps failed, then something bad is happening.  Maybe we should
> -     issue a message about a possible security attack in progress?  */
> +  /* mkstemps failed. It may be EPERM, ENOSPC etc. */
>    if (fd == -1)
> -    abort ();
> -  /* Similarly if we can not close the file.  */
> +    {
> +      fprintf(stderr, "Cannot create temporary file in %s: %s\n", base, strerror(errno));
> +      abort ();
> +    }
>    if (close (fd))
> -    abort ();
> +    abort (); /* sheer paranoia */
>    return temp_filename;
>  }

make_temp_file is documented as returning NULL on failure.  Would it
be reasonable to simply return NULL for certain errno codes?

As far as the patch goes:
* Comments should be complete sentences.
* The fprintf line is too long--it needs to wrap at 80 columns.
* Don't put a comment on the same line as abort ().

Otherwise this seems good.  How did you test it?

Ian

Re: [PATCH] gcc cannot deal with full /tmp

[Aaron W. LaFramboise]

Joseph S. Myers wrote:
> On Mon, 28 Jul 2008, Denys Vlasenko wrote:

>> This is a trivial fix for
>> https://bugzilla.redhat.com/show_bug.cgi?id=203231

> This probably needs testing for MinGW; Windows has its own error reporting 
> mechanisms and I don't know if you'll get the desired POSIX errno values.

Thanks for pointing this out Joseph.

I did not actually test this code; however I manually tested that open() 
with the options that libiberty mkstemps() uses will generate EEXIST if 
the file already exists, and not in various other conditions I tested. 
Also, the documentation 
<http://msdn.microsoft.com/en-us/library/z0kc8e3z(VS.71).aspx> seems to 
indicate this will work.

So I think this is correct for MinGW.

Re: [PATCH] gcc cannot deal with full /tmp

[Joseph S. Myers]

On Mon, 28 Jul 2008, Denys Vlasenko wrote:

> Hi,
> 
> This is a trivial fix for
> https://bugzilla.redhat.com/show_bug.cgi?id=203231
> 
> This bug is actually in libiberty, thus I post the patch to both gcc and
> binutils list.
> 
> Bug description:
> mkstemps() loops TMP_MAX times even if open error is fatal. This creates
> an impression that gcc hung (TMP_MAX is ~200000). Moreover, when it does
> finish looping and returns to make_temp_file(), make_temp_file()
> just abort()s without good error message.
> 
> The patch deals with both problems.

This probably needs testing for MinGW; Windows has its own error reporting 
mechanisms and I don't know if you'll get the desired POSIX errno values.

-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH] gcc cannot deal with full /tmp

[Denys Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

Hi,

This is a trivial fix for
https://bugzilla.redhat.com/show_bug.cgi?id=203231

This bug is actually in libiberty, thus I post the patch to both gcc and
binutils list.

Bug description:
mkstemps() loops TMP_MAX times even if open error is fatal. This creates
an impression that gcc hung (TMP_MAX is ~200000). Moreover, when it does
finish looping and returns to make_temp_file(), make_temp_file()
just abort()s without good error message.

The patch deals with both problems.

Sample output:

Cannot create temporary file in /root/srcdevel/gcc/testdir/: Permission denied
./z.sh: line 13:  6115 Aborted                 TMP=$PWD/testdir i486-linux-uclibc-gcc -o t t.c



Please ACK or comment. Thanks.
--
vda


[-- Attachment #2: 1.patch --]
[-- Type: text/x-diff, Size: 1666 bytes --]

diff -d -urpN gcc.0/libiberty/make-temp-file.c gcc.1/libiberty/make-temp-file.c
--- gcc.0/libiberty/make-temp-file.c	2008-07-21 14:50:00.000000000 +0200
+++ gcc.1/libiberty/make-temp-file.c	2008-07-21 16:28:06.000000000 +0200
@@ -23,6 +23,7 @@ Boston, MA 02110-1301, USA.  */
 
 #include <stdio.h>	/* May get P_tmpdir.  */
 #include <sys/types.h>
+#include <errno.h>
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>
 #endif
@@ -166,12 +167,13 @@ make_temp_file (const char *suffix)
   strcpy (temp_filename + base_len + TEMP_FILE_LEN, suffix);
 
   fd = mkstemps (temp_filename, suffix_len);
-  /* If mkstemps failed, then something bad is happening.  Maybe we should
-     issue a message about a possible security attack in progress?  */
+  /* mkstemps failed. It may be EPERM, ENOSPC etc. */
   if (fd == -1)
-    abort ();
-  /* Similarly if we can not close the file.  */
+    {
+      fprintf(stderr, "Cannot create temporary file in %s: %s\n", base, strerror(errno));
+      abort ();
+    }
   if (close (fd))
-    abort ();
+    abort (); /* sheer paranoia */
   return temp_filename;
 }
diff -d -urpN gcc.0/libiberty/mkstemps.c gcc.1/libiberty/mkstemps.c
--- gcc.0/libiberty/mkstemps.c	2008-07-21 14:50:00.000000000 +0200
+++ gcc.1/libiberty/mkstemps.c	2008-07-28 18:48:34.000000000 +0200
@@ -127,6 +127,9 @@ mkstemps (char *pattern, int suffix_len)
       if (fd >= 0)
 	/* The file does not exist.  */
 	return fd;
+      if (errno != EEXIST)
+	/* Fatal error (EPERM, ENOSPC etc). Doesn't make sense to loop.  */
+	break;
 
       /* This is a random value.  It is only necessary that the next
 	 TMP_MAX values generated by adding 7777 to VALUE are different