Re: [PATCH 1/3 v2] uname: use only one OSVERSIONINFOEX struct

[Bruno Haible]

Paolo Bonzini wrote, trying to reduce the number of system calls:
> --- a/lib/uname.c
> +++ b/lib/uname.c
> @@ -34,7 +34,7 @@
>  int
>  uname (struct utsname *buf)
>  {
> -  OSVERSIONINFO version;
> +  OSVERSIONINFOEX version;
>    BOOL have_version;
>    const char *super_version;
>  
> @@ -42,9 +42,18 @@ uname (struct utsname *buf)
>    if (gethostname (buf->nodename, sizeof (buf->nodename)) < 0)
>      strcpy (buf->nodename, "localhost");
>  
> -  /* Determine major-major Windows version.  */
> -  version.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
> -  have_version = GetVersionEx (&version);
> +  /* Determine major-minor Windows version.  */
> +  version.dwOSVersionInfoSize = sizeof (OSVERSIONINFOEX);
> +  have_version = GetVersionEx ((OSVERSIONINFO *) &version);
> +  if (!have_version)
> +    {
> +      /* OSVERSIONINFOEX info not available, live with OSVERSIONINFO which
> +      is a subset.  */
> +      memset (&version, 0, sizeof (version));
> +      version.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
> +      have_version = GetVersionEx ((OSVERSIONINFO *) &version);
> +    }
> +
>    if (have_version)
>      {
>        if (version.dwPlatformId == VER_PLATFORM_WIN32_NT)

With this hunk you introduce a pitfall to the next maintainer: it makes
it too easy to access fields of OSVERSIONINFOEX that have not been fully
initialized.

> @@ -129,11 +138,7 @@ uname (struct utsname *buf)
>             }
>         else if (version.dwMajorVersion == 6)
>           {
> -           OSVERSIONINFOEX versionex;
> -
> -           versionex.dwOSVersionInfoSize = sizeof (OSVERSIONINFOEX);
> -           if (GetVersionEx ((OSVERSIONINFO *) &versionex)
> -               && versionex.wProductType != VER_NT_WORKSTATION)
> +           if (version.wProductType != VER_NT_WORKSTATION)
>               strcpy (buf->release, "Windows Server 2008");
>             else
>               switch (version.dwMinorVersion)

And here you do exactly this: You access 'version.wProductType' without
having verified that this field was initialized by the first GetVersionEx
call. The memset call above is a way to silence valgrind, but who tells
you that the zero values filled in by memset will be interpreted in the
right way?

This is too dangerous coding, in my opinion. At least keep a 'have_versionex'
variable that tells whether the OSVERSIONINFOEX struct has been filled. I'm
committing this:


2009-10-03  Paolo Bonzini  <bonz...@gnu.org>
            Bruno Haible  <br...@clisp.org>

        * lib/uname.c: Include <string.h>.
        (uname): Do only one call to GetVersionEx in the common case.

--- lib/uname.c.orig    2009-10-03 20:32:48.000000000 +0200
+++ lib/uname.c 2009-10-03 20:31:47.000000000 +0200
@@ -24,6 +24,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>
 #include <windows.h>
 
@@ -49,16 +50,31 @@
 uname (struct utsname *buf)
 {
   OSVERSIONINFO version;
+  OSVERSIONINFOEX versionex;
+  BOOL have_versionex; /* indicates whether versionex is filled */
   const char *super_version;
 
+  /* Preparation: Fill version and, if possible, also versionex.
+     But try to call GetVersionEx only once in the common case.  */
+  versionex.dwOSVersionInfoSize = sizeof (OSVERSIONINFOEX);
+  have_versionex = GetVersionEx (&versionex);
+  if (have_versionex)
+    {
+      /* We know that OSVERSIONINFO is a subset of OSVERSIONINFOEX.  */
+      memcpy (&version, &versionex, sizeof (OSVERSIONINFO));
+    }
+  else
+    {
+      version.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
+      if (!GetVersionEx (&version))
+       abort ();
+    }
+
   /* Fill in nodename.  */
   if (gethostname (buf->nodename, sizeof (buf->nodename)) < 0)
     strcpy (buf->nodename, "localhost");
 
   /* Determine major-major Windows version.  */
-  version.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
-  if (!GetVersionEx (&version))
-    abort ();
   if (version.dwPlatformId == VER_PLATFORM_WIN32_NT)
     {
       /* Windows NT or newer.  */
@@ -135,11 +151,7 @@
          }
       else if (version.dwMajorVersion == 6)
        {
-         OSVERSIONINFOEX versionex;
-
-         versionex.dwOSVersionInfoSize = sizeof (OSVERSIONINFOEX);
-         if (GetVersionEx ((OSVERSIONINFO *) &versionex)
-             && versionex.wProductType != VER_NT_WORKSTATION)
+         if (have_versionex && versionex.wProductType != VER_NT_WORKSTATION)
            strcpy (buf->release, "Windows Server 2008");
          else
            switch (version.dwMinorVersion)