Re: [ros-diffs] [weiden] 16536: return the length of the string excluding the null-termination character on success in GetEnvironmentVariable(). Thanks to Hartmut.

[Joseph Galbraith]

It seems like there is some uncertainty between this
code and RtlQueryEnvironmentVariable_U about who is
going to ensure that the string returned from
GetEnvironmentVariable() is nul terminated...

If I read the code in RtlQueryEnvironmentVariable_U
correctly (and I don't swear I have), it is possible
for it to return STATUS_SUCCESS and not copy a
NUL terminator.  This happens if the buffer is large
enough to contain the value but not the NUL terminator.

This is probably correct for this function since
UNICODE_STRINGs (and the Rtl* library) generally have
no requirement of a NUL terminator.

But then, if I read the code in GetEnvironmentVariable()
correctly, I don't believe it takes this situation into
account.

Probably, it would be best if RtlQueryEnvironmentVariable_U
_never_ copied the NUL terminator, and the caller was
always responsible for adding NUL termination, if needed.

Thanks,

Joseph

PS. MSDN is slightly unclear (to me) on how it should behave
in this case, but it seems unlikely that the real
GetEnvironmentVariable() returns success and a non-nul-terminated
string.  But I haven't written a test program to prove it.


weiden(a)svn.reactos.com wrote:
>   return the length of the string excluding the
> null-termination character on success in GetEnvironmentVariable(). Thanks to Hartmut.
>  
>  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-11 18:22:53 UTC (rev 16535)
>  +++ trunk/reactos/lib/kernel32/misc/env.c	2005-07-11 20:30:33 UTC (rev 16536)
>  @@ -91,7 +91,7 @@
>  
>   	/* free unicode variable name string */
>   	RtlFreeUnicodeString (&VarNameU);
>   
>  
>  -	return (VarValueU.Length / sizeof(WCHAR) + 1);
>  
>  +	return (VarValueU.Length / sizeof(WCHAR));
>  
>   }
>   
>   
>  @@ -133,7 +133,7 @@
>  
>   		}
>   	}
>   
>  
>  -	return (VarValue.Length / sizeof(WCHAR) + 1);
>  
>  +	return (VarValue.Length / sizeof(WCHAR));
>  
>   }
>   
>   
>

[Thomas Weidenmueller]

Joseph Galbraith wrote:
>   If I read the code in RtlQueryEnvironmentVariable_U
>  correctly (and I don't swear I have), it is possible
>  for it to return STATUS_SUCCESS and not copy a
>  NUL terminator.  This happens if the buffer is large
>  enough to contain the value but not the NUL terminator. 
That is true and needs to be fixed.

>   But then, if I read the code in
> GetEnvironmentVariable()
>  correctly, I don't believe it takes this situation into
>  account. 
Agreed, see above.

>   Probably, it would be best if
> RtlQueryEnvironmentVariable_U
>  _never_ copied the NUL terminator, and the caller was
>  always responsible for adding NUL termination, if needed. 
No, it has to but it depends on whether there's enough space.

My proposal to fix the issue is that we change:

    VarValue.MaximumLength = nSize * sizeof(WCHAR);

in GetEnvironmentVariableW to:

    VarValue.MaximumLength = (nSize - 1) * sizeof(WCHAR);

Of course, similar changes would have to be applied to the ansi version.
However, the case where nSize == 0 (and maybe also 1) would have to be
handled appropriately. This way (given we set the termination character
after the RtlQueryEnvironmentVariable_U call) we'd ensure the string is
always NULL-terminated because RtlQueryEnvironmentVariable_U only
terminates it if MaximumLength > Length.

Best Regards,
Thomas