[PATCH] Libiberty for VMS - mkstemps.c don't mix case

[PATCH] Libiberty for VMS - mkstemps.c don't mix case

[Douglas B Rupp]

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

VMS temp files must be case insensitive.

Tested on x86-64-linux-gnu

--Douglas B Rupp
AdaCore


[-- Attachment #2: 12.2libiberty.dif.txt --]
[-- Type: text/plain, Size: 1996 bytes --]

2009-07-22  Douglas B Rupp  <rupp@gnat.com>

	* mkstemps.c (mkstemps): Don't use mixed case on VMS.

diff -rupN libiberty/mkstemps.c libiberty/mkstemps.c
--- libiberty/mkstemps.c	2008-07-31 14:22:09.000000000 -0700
+++ libiberty/mkstemps.c	2009-01-21 14:40:34.000000000 -0800
@@ -77,8 +77,8 @@ reading and writing.
 int
 mkstemps (char *pattern, int suffix_len)
 {
-  static const char letters[]
-    = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
+  char *letters;
+  int nletters = 0;
   static gcc_uint64_t value;
 #ifdef HAVE_GETTIMEOFDAY
   struct timeval tv;
@@ -87,6 +87,21 @@ mkstemps (char *pattern, int suffix_len)
   size_t len;
   int count;
 
+  if (nletters == 0)
+    {
+      char *gletters = getenv ("GNU_MKSTEMPS_LETTERS");
+      if (gletters)
+        letters = strdup (gletters);
+      else
+        letters = strdup
+#ifdef VMS
+          ("abcdefghijklmnopqrstuvwxyz0123456789");
+#else
+          ("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789");
+#endif
+      nletters = (sizeof letters) - 1;
+    }
+
   len = strlen (pattern);
 
   if ((int) len < 6 + suffix_len
@@ -111,17 +126,17 @@ mkstemps (char *pattern, int suffix_len)
       int fd;
 
       /* Fill in the random bits.  */
-      XXXXXX[0] = letters[v % 62];
-      v /= 62;
-      XXXXXX[1] = letters[v % 62];
-      v /= 62;
-      XXXXXX[2] = letters[v % 62];
-      v /= 62;
-      XXXXXX[3] = letters[v % 62];
-      v /= 62;
-      XXXXXX[4] = letters[v % 62];
-      v /= 62;
-      XXXXXX[5] = letters[v % 62];
+      XXXXXX[0] = letters[v % nletters];
+      v /= nletters;
+      XXXXXX[1] = letters[v % nletters];
+      v /= nletters;
+      XXXXXX[2] = letters[v % nletters];
+      v /= nletters;
+      XXXXXX[3] = letters[v % nletters];
+      v /= nletters;
+      XXXXXX[4] = letters[v % nletters];
+      v /= nletters;
+      XXXXXX[5] = letters[v % nletters];
 
       fd = open (pattern, O_BINARY|O_RDWR|O_CREAT|O_EXCL, 0600);
       if (fd >= 0)

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[DJ Delorie]

> VMS temp files must be case insensitive.

So do Windows files, but they haven't complained yet...  Why is VMS
different?

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[Douglas B Rupp]

DJ Delorie wrote:
>> VMS temp files must be case insensitive.
> 
> So do Windows files, but they haven't complained yet...  Why is VMS
> different?
> 

Well... just because windows users haven't complained doesn't mean the 
problem doesn't theoretically exist. I would argue VMS machines are 
genrally larger with more simultaneous developers than windows and so 
the problem is more likely to occur.

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[DJ Delorie]

> Well... just because windows users haven't complained doesn't mean the 
> problem doesn't theoretically exist. I would argue VMS machines are 
> genrally larger with more simultaneous developers than windows and so 
> the problem is more likely to occur.

I would argue that we already check for file conflicts, and don't need
to worry about it.

      fd = open (pattern, O_BINARY|O_RDWR|O_CREAT|O_EXCL, 0600);
      if (fd >= 0)
	/* The file does not exist.  */
	return fd;
      if (errno != EEXIST

If VMS is truly case insensitive, wouldn't it detect such conflicts
via the above O_EXCL code?

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[Douglas B Rupp]

DJ Delorie wrote:
>> Well... just because windows users haven't complained doesn't mean the 
>> problem doesn't theoretically exist. I would argue VMS machines are 
>> genrally larger with more simultaneous developers than windows and so 
>> the problem is more likely to occur.
> 
> I would argue that we already check for file conflicts, and don't need
> to worry about it.
> 
>       fd = open (pattern, O_BINARY|O_RDWR|O_CREAT|O_EXCL, 0600);
>       if (fd >= 0)
> 	/* The file does not exist.  */
> 	return fd;
>       if (errno != EEXIST
> 
> If VMS is truly case insensitive, wouldn't it detect such conflicts
> via the above O_EXCL code?
> 

VMS has file versioning, I think it would detect it only if the version 
was specified or the directory as created with single versioning.

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[DJ Delorie]

> VMS has file versioning, I think it would detect it only if the
> version was specified or the directory was created with single
> versioning.

Doh!  I forgot about that.

Ok, that leads to a new question - shouldn't we specify a version, or
single versioning, when creating temp files, so that someone else
can't just create a new copy of the same file with a newer version and
trip up the compiler?  Or do the file permissions pass on to new
versions?

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[Douglas B Rupp]

DJ Delorie wrote:
>> VMS has file versioning, I think it would detect it only if the
>> version was specified or the directory was created with single
>> versioning.
> 
> Doh!  I forgot about that.
> 
> Ok, that leads to a new question - shouldn't we specify a version, or
> single versioning, when creating temp files, so that someone else
> can't just create a new copy of the same file with a newer version and
> trip up the compiler?  Or do the file permissions pass on to new
> versions?
> 

That's what I'm thinking needs to happen. I need to research the history 
  of why I put this in to our sources.

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[Douglas B Rupp]

DJ Delorie wrote:
>> VMS has file versioning, I think it would detect it only if the
>> version was specified or the directory was created with single
>> versioning.
> 
> Doh!  I forgot about that.
> 
> Ok, that leads to a new question - shouldn't we specify a version, or
> single versioning, when creating temp files, so that someone else
> can't just create a new copy of the same file with a newer version and
> trip up the compiler?  Or do the file permissions pass on to new
> versions?
> 

At long last getting back to this discussion.  I did some experiments 
with forcing the temp file to have a version number for the .s temp file 
created by gcc and passed to cc1.  Creating the temp file works fine, 
but cc1 now gets the fatal error:
"can't open <sometempdir>/cccadaaa.s.1 for writing: file exists"

The problem comes from toplev.c/init_asm_output
asm_out_file = fopen (asm_file_name, "w+b");

The "w+" attribute says open a new file for writing. The file exists, 
the filename has a version number on it already, so VMS generates this 
error.

If I change the attributes to "a+b" it works fine on VMS.

So what now?  Seems to me the VMS behavior is entirely rational in 
insisting on a new version and that the Unix behavior is somewhat 
arbitrary in ignoring the fact the file already exists.

What is your thinking?

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[Douglas B Rupp]

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

Douglas B Rupp wrote:
> What is your thinking?
> 

For the sake of discussion, this is the patch.




[-- Attachment #2: wb.dif.txt --]
[-- Type: text/plain, Size: 1885 bytes --]

diff -rupN gcc-head-src.orig/gcc/gcc.c gcc-head-src/gcc/gcc.c
--- gcc-head-src.orig/gcc/gcc.c	2010-06-24 17:26:24.000000000 -0700
+++ gcc-head-src/gcc/gcc.c	2010-06-27 15:22:34.000000000 -0700
@@ -5386,7 +5386,27 @@ do_spec_1 (const char *spec, int inswitc
 			saved_suffix = NULL;
 		      }
 		    else
-		      t->suffix = save_string (suffix, suffix_length);
+		      {
+#ifdef VMS
+			/* Force an explicit version number on VMS.  */
+			char *vmssuffix = alloca (suffix_length + 4);
+
+			strncpy (vmssuffix, suffix, suffix_length);
+			if (suffix_length > 0)
+			  {
+			    strncpy (vmssuffix + suffix_length, ".1", 3);
+			    suffix_length += 2;
+			  }
+			else
+			  {
+			    strncpy (vmssuffix + suffix_length, "..1", 4);
+			    suffix_length += 3;
+			  }
+			suffix = vmssuffix;
+#endif
+			t->suffix = save_string (suffix, suffix_length);
+		      }
+
 		    t->unique = (c == 'u' || c == 'U' || c == 'j');
 		    temp_filename = make_temp_file (t->suffix);
 		    temp_filename_length = strlen (temp_filename);
diff -rupN gcc-head-src.orig/gcc/toplev.c gcc-head-src/gcc/toplev.c
--- gcc-head-src.orig/gcc/toplev.c	2010-06-20 14:02:46.000000000 -0700
+++ gcc-head-src/gcc/toplev.c	2010-06-27 15:23:41.000000000 -0700
@@ -1382,9 +1382,22 @@ init_asm_output (const char *name)
       if (!strcmp (asm_file_name, "-"))
 	asm_out_file = stdout;
       else
-	asm_out_file = fopen (asm_file_name, "w+b");
+	asm_out_file = fopen (asm_file_name, "a+b");
       if (asm_out_file == 0)
 	fatal_error ("can%'t open %s for writing: %m", asm_file_name);
+
+      if (asm_out_file != stdout)
+	{
+	  int fd, ierr;
+
+	  fd = fileno (asm_out_file);
+	  if (fd < 0)
+	    fatal_error ("can%'t get fileno for %s: %m", asm_file_name);
+
+	  ierr = ftruncate (fd, 0);
+	  if (ierr < 0)
+	    fatal_error ("can%'t truncate %s: %m", asm_file_name);
+	}
     }
 
   if (!flag_syntax_only)

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[DJ Delorie]

As a target maintainer, it's up to you to decide what the security
risks are, and how relevent they are to your platform.  My job is to
make sure you've thought of them :-)

If we can come up with a generic solution that works right for all
host environments, that's great.  If not, you get to decide how much
hacking you want to do for yours.  I think you'll get arguments if you
expect everyone else to assume their environment is not posix, though.

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[Douglas B Rupp]

DJ Delorie wrote:
> As a target maintainer, it's up to you to decide what the security
> risks are, and how relevent they are to your platform.  My job is to
> make sure you've thought of them :-)
> 
> If we can come up with a generic solution that works right for all
> host environments, that's great.  If not, you get to decide how much
> hacking you want to do for yours.  I think you'll get arguments if you
> expect everyone else to assume their environment is not posix, though.
>

Right, maybe you can help me understand the risks on VMS.

Going back to the existing behavior make_temp_file will create a new 
file since the test for existence is not version specific, it will ways 
be version 1.  Toplev.c in w+b mode with create a new file with the same 
name, but with version 2 and output the assembly.  Gas, when called will 
open the newest version file for it's input.

What is the risk?  Well seems like a hacker could insert a version 3 of 
the .s file during the time cc1 is working and gas would not be the 
wiser.  Cc1 or Gnat1 could potentially run for a long time so there's 
significant window of opportunity.

Am I thinking about this correctly?

--Doug

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[DJ Delorie]

> Well seems like a hacker could insert a version 3 of the .s file
> during the time cc1 is working and gas would not be the wiser.

Yes, this is the risk.  The new .o file could, when linked and
executed, give admin access to an attacker.

Re: [PATCH] Libiberty for VMS - mkstemps.c don't mix case

[Douglas B Rupp]

Douglas B Rupp wrote:
> DJ Delorie wrote:
>>> Well... just because windows users haven't complained doesn't mean 
>>> the problem doesn't theoretically exist. I would argue VMS machines 
>>> are genrally larger with more simultaneous developers than windows 
>>> and so the problem is more likely to occur.
>>
>> I would argue that we already check for file conflicts, and don't need
>> to worry about it.
>>
>>       fd = open (pattern, O_BINARY|O_RDWR|O_CREAT|O_EXCL, 0600);
>>       if (fd >= 0)
>>     /* The file does not exist.  */
>>     return fd;
>>       if (errno != EEXIST
>>
>> If VMS is truly case insensitive, wouldn't it detect such conflicts
>> via the above O_EXCL code?
>>
> 
> VMS has file versioning, I think it would detect it only if the version 
> was specified or the directory as created with single versioning.
> 

There's an obvious flaw in that argument. I'll withdraw the patch for 
now pending more testing.