Re: [PATCH] supports_delete_on_close

[Bruno Haible]

Hi John,

> I'm submitting this patch which Windows users have confirmed fixes a bug
> where tempfiles are not deleted.  This was caused by failing to initialise 
> the argument to GetVersionEx, like the MS documentation mandates.

Oops, a big blunder, as it could cause stack corruption if the uninitialized
value of v.dwOSVersionInfoSize happens to be == sizeof (OSVERSIONINFOEX).

> It seems that there are two identical copies (with the identical bug) of
> supports_delete_on_close in gnulib, one in clean-temp.c and the other in
> tmpfile.c

Yup, right. And in the other 2 places (w32spawn.h and uname.c) it's done
right.

> I know you guys don't like fixes for bugs until they've
> actually been reported to cause a problem to end users

It depends. We like to get notified about bugs in our code in all cases.
Often it does take an actual problem report to make it evident that there
is a bug. But here it's an evident misuse of a system API; no need to
try to exhibit its bad effects, as it's so obvious.

> (but my recommendation
> FWIW would be to refactor these two functions into a single public one).

This would mean that we have a function that is exported API of a module
but exists only on certain platforms. While this is not entirely impossible,
we generally try to design a module's API so that it is platform independent.
Of course the size of the duplicated role is also relevant; here it's
just 20 lines of code, with very simple logic.

I'm applying the patch below in your name. I took the freedom to insert
a line break and shorten the URL.


2012-06-20  John Darrington  <address@hidden>  (tiny change)

        tmpfile, clean-temp: Fix invocation of GetVersionEx.
        * lib/tmpfile.c (supports_delete_on_close): Initialize parameter for
        GetVersionEx correctly.
        * lib/clean-temp.c (supports_delete_on_close): Likewise.

*** lib/clean-temp.c.orig       Wed Jun 20 23:30:47 2012
--- lib/clean-temp.c    Wed Jun 20 23:28:12 2012
***************
*** 583,588 ****
--- 583,593 ----
      {
        OSVERSIONINFO v;
  
+       /* According to
+          
<http://msdn.microsoft.com/en-us/library/windows/desktop/ms724451(v=vs.85).aspx>
+          this structure must be initialised as follows:  */
+       v.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
+ 
        if (GetVersionEx (&v))
          known = (v.dwPlatformId == VER_PLATFORM_WIN32_NT ? 1 : -1);
        else
*** lib/tmpfile.c.orig  Wed Jun 20 23:30:47 2012
--- lib/tmpfile.c       Wed Jun 20 23:28:12 2012
***************
*** 54,59 ****
--- 54,64 ----
      {
        OSVERSIONINFO v;
  
+       /* According to
+          
<http://msdn.microsoft.com/en-us/library/windows/desktop/ms724451(v=vs.85).aspx>
+          this structure must be initialised as follows:  */
+       v.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
+ 
        if (GetVersionEx (&v))
          known = (v.dwPlatformId == VER_PLATFORM_WIN32_NT ? 1 : -1);
        else