[weiden] 16553: handle memory allocation errors in GetEnvironmentVariable() and ensure the string is always null-terminated

handle memory allocation errors in GetEnvironmentVariable() and ensure
the string is always null-terminated
Modified: trunk/reactos/lib/kernel32/misc/env.c
  _____

Modified: trunk/reactos/lib/kernel32/misc/env.c
--- trunk/reactos/lib/kernel32/misc/env.c	2005-07-13 08:48:49 UTC
(rev 16552)
+++ trunk/reactos/lib/kernel32/misc/env.c	2005-07-13 15:09:52 UTC
(rev 16553)
@@ -48,17 +48,33 @@

/* initialize unicode variable value string and allocate buffer
*/
    VarValueU.Length = 0;
-	VarValueU.MaximumLength = nSize * sizeof(WCHAR);
-	VarValueU.Buffer = RtlAllocateHeap (RtlGetProcessHeap (),
-	                                    0,
-	                                    VarValueU.MaximumLength);
+	if (nSize != 0)
+	{
+	    VarValueU.MaximumLength = (nSize - 1) * sizeof(WCHAR);
+	    VarValueU.Buffer = RtlAllocateHeap (RtlGetProcessHeap (),
+	                                        0,
+	                                        nSize * sizeof(WCHAR));
+            if (VarValueU.Buffer != NULL)
+            {
+                /* NULL-terminate the buffer in any case!
RtlQueryEnvironmentVariable_U
+                   only terminates it if MaximumLength < Length! */
+                VarValueU.Buffer[nSize - 1] = L'\0';
+            }
+        }
+	else
+	{
+            VarValueU.MaximumLength = 0;
+            VarValueU.Buffer = NULL;
+	}

-	/* get unicode environment variable */
-	Status = RtlQueryEnvironmentVariable_U (NULL,
-	                                        &VarNameU,
-	                                        &VarValueU);
-	if (!NT_SUCCESS(Status))
-	{
+        if (VarValueU.Buffer != NULL || nSize == 0)
+        {
+            /* get unicode environment variable */
+	    Status = RtlQueryEnvironmentVariable_U (NULL,
+	                                            &VarNameU,
+	                                            &VarValueU);
+	    if (!NT_SUCCESS(Status))
+	    {
    	/* free unicode buffer */
    	RtlFreeHeap (RtlGetProcessHeap (),
    	             0,
@@ -70,28 +86,37 @@
    	SetLastErrorByStatus (Status);
    	if (Status == STATUS_BUFFER_TOO_SMALL)
    	{
-			return VarValueU.Length / sizeof(WCHAR) + 1;
+			return (VarValueU.Length / sizeof(WCHAR)) + 1;
    	}
    	else
    	{
    		return 0;
    	}
-	}
+	    }

-	/* convert unicode value string to ansi */
-	RtlUnicodeStringToAnsiString (&VarValue,
-	                              &VarValueU,
-	                              FALSE);
+	    /* convert unicode value string to ansi */
+	    RtlUnicodeStringToAnsiString (&VarValue,
+	                                  &VarValueU,
+	                                  FALSE);

-	/* free unicode buffer */
-	RtlFreeHeap (RtlGetProcessHeap (),
-	             0,
-	             VarValueU.Buffer);
+            if (VarValueU.Buffer != NULL)
+            {
+                /* free unicode buffer */
+	        RtlFreeHeap (RtlGetProcessHeap (),
+	                     0,
+	                     VarValueU.Buffer);
+            }

-	/* free unicode variable name string */
-	RtlFreeUnicodeString (&VarNameU);
+	    /* free unicode variable name string */
+	    RtlFreeUnicodeString (&VarNameU);

-	return (VarValueU.Length / sizeof(WCHAR));
+	    return (VarValueU.Length / sizeof(WCHAR));
+        }
+        else
+        {
+            SetLastError (ERROR_NOT_ENOUGH_MEMORY);
+            return 0;
+        }
 }

@@ -114,7 +139,7 @@
                          lpName);

VarValue.Length = 0;
-	VarValue.MaximumLength = nSize * sizeof(WCHAR);
+	VarValue.MaximumLength = (nSize != 0 ? (nSize - 1) *
sizeof(WCHAR) : 0);
    VarValue.Buffer = lpBuffer;

Status = RtlQueryEnvironmentVariable_U (NULL,
@@ -125,13 +150,17 @@
    	SetLastErrorByStatus (Status);
    	if (Status == STATUS_BUFFER_TOO_SMALL)
    	{
-			return (VarValue.Length / sizeof(WCHAR)) +  1;
+			return (VarValue.Length / sizeof(WCHAR)) + 1;
    	}
    	else
    	{
    		return 0;
    	}
    }
+	
+	/* make sure the string is NULL-terminated!
RtlQueryEnvironmentVariable_U
+           only terminates it if MaximumLength < Length */
+	VarValue.Buffer[VarValue.Length / sizeof(WCHAR)] = L'\0';

return (VarValue.Length / sizeof(WCHAR));
 }

[Joseph Galbraith]

> VarValue.MaximumLength = (nSize != 0 ? (nSize - 1) * sizeof(WCHAR) : 0);
>
> VarValue.Buffer = lpBuffer;
>
> Status = RtlQueryEnvironmentVariable_U (NULL,
>
> @@ -125,13 +150,17 @@
> SetLastErrorByStatus (Status);
> if (Status == STATUS_BUFFER_TOO_SMALL)
> {
>
> 	return (VarValue.Length / sizeof(WCHAR)) +  1;
>
> 	return (VarValue.Length / sizeof(WCHAR)) + 1;
>
> }
> else
> {
> 	return 0;
> }
> }
>
> /* make sure the string is NULL-terminated! RtlQueryEnvironmentVariable_U
>       only terminates it if MaximumLength < Length */
>
> VarValue.Buffer[VarValue.Length / sizeof(WCHAR)] = L'\0';

Is it possible for an environment variable's value to be empty?

If it is, RtlQueryEnvironmentVariable_U() could succeeded even
if a zero length buffer is passed in.  And then, would we crash
appending the NUL termination?

Since GetEnvironmentVariable has to have a buffer of at least
one character to succeed, maybe we should add such a guard to
the top of the function...

Thanks,

Joseph

[Joseph Galbraith]

Joseph Galbraith wrote:
> Is it possible for an environment variable's value to be empty?

Sorry... didn't mean for this to go both places... argh.

Thanks,

Joseph