Make 3.99.91/92 - Output-Synchronization fails with UAC or Non-Admins (under Windows)

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

Make 3.99.91/92 - Output-Synchronization fails with UAC or Non-Admins (under Windows)

Markus Eisenmann
Hi!

I have built a windows-make version of the release-canditate versions 3.99.91 and 3.99.92,
using Visual Studio and MinGW. The result, I.e. make fails if I'm using parallel builds and the featureĀ  'output-synchronization' with an error like 'could not create tmpfile'.

After spending some time for debugging, I have detected the "problem":
- A temporary file is used to buffer the outputs (of course)
- this file is created with calling 'tmpfile()'.

tmpfile() does create a temporary file in the root directory; But in many cases - e.g. inhibited by NTFS-security or if UAC is active - a creation of files in the root-directory is forbidden.

IMHO, I think, this type of temporary files should be created like other needed temporary (batch-) files in the TMP/TEMP-folder (using the associated environment variable).


Best regards from Salzburg,
Markus


_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Make 3.99.91/92 - Output-Synchronization fails with UAC or Non-Admins (under Windows)

Eli Zaretskii
> Date: Mon, 23 Sep 2013 16:21:30 +0200
> From: Markus Eisenmann <[hidden email]>
>
> I have built a windows-make version of the release-canditate versions
> 3.99.91 and 3.99.92,
> using Visual Studio and MinGW. The result, I.e. make fails if I'm using
> parallel builds and the feature  'output-synchronization' with an error
> like 'could not create tmpfile'.
>
> After spending some time for debugging, I have detected the "problem":
> - A temporary file is used to buffer the outputs (of course)
> - this file is created with calling 'tmpfile()'.
>
> As written in
> http://msdn.microsoft.com/en-us/library/x8x7sakw%28v=vs.90%29.aspx,
> tmpfile() does create a temporary file in the root directory; But in many
> cases - e.g. inhibited by NTFS-security or if UAC is active - a creation of
> files in the root-directory is forbidden.

I don't think this has anything to do with UAC, it's just that the
root of a system disk is write-protected from casual writes.  How
silly of Microsoft not to do something about this on Windows 7 and
later!

> IMHO, I think, this type of temporary files should be created like other
> needed temporary (batch-) files in the TMP/TEMP-folder (using the
> associated environment variable).

I agree.  Can you please test with the following patch?  I'm also
testing it; if it will give good results, I will commit it to the
repository.

Thanks.

--- output.c~0 2013-09-23 00:10:34.000000000 +0300
+++ output.c 2013-09-23 22:58:51.963375000 +0300
@@ -72,6 +72,103 @@ msc_vsnprintf (char *str, size_t size, c
 }
 #endif
 
+#ifdef WINDOWS32
+/* A replacement for tmpfile, since the MSVCRT implementation creates
+   the file in the root directory of the current drive, which might
+   not be writable by our user.  Most of the code borrowed from
+   create_batch_file, see job.c.  */
+FILE *
+tmpfile (void)
+{
+  char temp_path[MAXPATHLEN];
+  unsigned path_size = GetTempPath (sizeof temp_path, temp_path);
+  int path_is_dot = 0;
+  /* The following variable is static so we won't try to reuse a name
+     that was generated a little while ago, because that file might
+     not be on disk yet, since we use FILE_ATTRIBUTE_TEMPORARY below,
+     which tells the OS it doesn't need to flush the cache to disk.
+     If the file is not yet on disk, we might think the name is
+     available, while it really isn't.  This happens in parallel
+     builds, where Make doesn't wait for one job to finish before it
+     launches the next one.  */
+  static unsigned uniq = 0;
+  static int second_loop = 0;
+  const char base[] = "make_tmpf";
+  const unsigned sizemax = sizeof base - 1 + 4 + 10 + 10;
+  unsigned pid = GetCurrentProcessId ();
+
+  if (path_size == 0)
+    {
+      path_size = GetCurrentDirectory (sizeof temp_path, temp_path);
+      path_is_dot = 1;
+    }
+
+  ++uniq;
+  if (uniq >= 0x10000 && !second_loop)
+    {
+      /* If we already had 64K batch files in this
+         process, make a second loop through the numbers,
+         looking for free slots, i.e. files that were
+         deleted in the meantime.  */
+      second_loop = 1;
+      uniq = 1;
+    }
+  while (path_size > 0 &&
+         path_size + sizemax < sizeof temp_path &&
+         !(uniq >= 0x10000 && second_loop))
+    {
+      sprintf (temp_path + path_size,
+       "%s%s%u-%x.tmp",
+       temp_path[path_size - 1] == '\\' ? "" : "\\",
+       base, pid, uniq);
+      /* O_CREAT | O_EXCL | O_RDWR | O_BINARY | O_TEMPORARY
+ SH_DENYNO */
+      HANDLE h = CreateFile (temp_path,  /* file name */
+                             GENERIC_READ | GENERIC_WRITE | DELETE, /* desired access */
+                             0,                            /* no share mode */
+                             NULL,                         /* default security attributes */
+                             CREATE_NEW,                   /* creation disposition */
+                             FILE_ATTRIBUTE_NORMAL |       /* flags and attributes */
+                             FILE_ATTRIBUTE_TEMPORARY |
+     FILE_FLAG_DELETE_ON_CLOSE,
+                             NULL);                        /* no template file */
+
+      if (h == INVALID_HANDLE_VALUE)
+        {
+          const DWORD er = GetLastError ();
+
+          if (er == ERROR_FILE_EXISTS || er == ERROR_ALREADY_EXISTS)
+            {
+              ++uniq;
+              if (uniq == 0x10000 && !second_loop)
+                {
+                  second_loop = 1;
+                  uniq = 1;
+                }
+            }
+
+          /* the temporary path is not guaranteed to exist */
+          else if (path_is_dot == 0)
+            {
+              path_size = GetCurrentDirectory (sizeof temp_path, temp_path);
+              path_is_dot = 1;
+            }
+
+          else
+    break;
+        }
+      else
+        {
+          int fd = _open_osfhandle ((intptr_t)h, 0);
+
+          return _fdopen (fd, "w+b");
+        }
+    }
+
+  return NULL;
+}
+#endif
+
 /* Write a string to the current STDOUT or STDERR.  */
 static void
 _outputs (struct output *out, int is_err, const char *msg)

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Make 3.99.91/92 - Output-Synchronization fails with UAC or Non-Admins (under Windows)

Paul Smith-20
On Mon, 2013-09-23 at 23:04 +0300, Eli Zaretskii wrote:
> > IMHO, I think, this type of temporary files should be created like
> other
> > needed temporary (batch-) files in the TMP/TEMP-folder (using the
> > associated environment variable).
>
> I agree.  Can you please test with the following patch?  I'm also
> testing it; if it will give good results, I will commit it to the
> repository.

I wonder if this new tmpfile() would be better added to a source file in
the w32 subdirectory somewhere, rather than ifdef'ing it into output.c?


_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Make 3.99.91/92 - Output-Synchronization fails with UAC or Non-Admins (under Windows)

Eli Zaretskii
> From: Paul Smith <[hidden email]>
> Cc: Markus Eisenmann <[hidden email]>, [hidden email]
> Date: Mon, 23 Sep 2013 16:38:36 -0400
>
> On Mon, 2013-09-23 at 23:04 +0300, Eli Zaretskii wrote:
> > > IMHO, I think, this type of temporary files should be created like
> > other
> > > needed temporary (batch-) files in the TMP/TEMP-folder (using the
> > > associated environment variable).
> >
> > I agree.  Can you please test with the following patch?  I'm also
> > testing it; if it will give good results, I will commit it to the
> > repository.
>
> I wonder if this new tmpfile() would be better added to a source file in
> the w32 subdirectory somewhere, rather than ifdef'ing it into output.c?

Sure, can do.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32
Reply | Threaded
Open this post in threaded view
|

Re: Make 3.99.91/92 - Output-Synchronization fails with UAC or Non-Admins (under Windows)

Eli Zaretskii
In reply to this post by Eli Zaretskii
> Date: Tue, 24 Sep 2013 07:34:44 +0200
> From: Markus Eisenmann <[hidden email]>
>
> Thank you for your answer!
>
> I will test ist maybe tomorrow (not enough time today).

I hope you did get to testing this.  I committed the changes to the
git repo.

_______________________________________________
Make-w32 mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/make-w32