On Wed, Sep 21, 2011 at 6:56 AM, matt d <mattjd at gmail dot com> wrote:
> Sure thing: https://bugs.php.net/bug.php?id=55749
>
> I think you'd need to check GetLastError() after the second call to
> GetEnvironmentVariableA, as that's how you can check for
> ERROR_ENVVAR_NOT_FOUND.

I could but it is not necessary. GetLastError only tells me why it fails, which 
is not that important here. But on failure (not found or other errors), the 
return value is zero. See http://msdn.microsoft.com/en-
us/library/windows/desktop/ms683188(v=vs.85).aspx

This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.

Thanks for the catch up!

See  http://svn.php.net/viewvc?view=revision&revision=317306

and http://svn.php.net/viewvc?view=revision&revision=317305 (committed in 5.4 by 
mistake with another fix).

Sorry for the late response, but what about this case:

From MSDN: "If lpBuffer is not large enough to hold the data, the return value is 
the buffer size, in characters, required to hold the string and its terminating 
null character and the contents of lpBuffer are undefined."

The environment variable might not only be removed/shortened/changed, but could be 
expanded as well.

That's nothing we can do about that, unless we do some kind of loops and pray that 
nothing changed in between :). Or do you have something in mind?

Actually I'm thinking about using fixed length buffer for all PATHs or other 
smaller than max path length values, and be done with that.

Having a loop that could take some time to complete in a core function like getenv 
doesn't sit well with me.

Other languages' implementations of the same feature appear to either only read 
the environment at startup, or rely on the user for thread safety.

Regardless of the choice made, we need to ensure that a garbage string is never 
returned.

it is never returned as of now.

I will simply fail if the returned length is not the same than the one we expect. 
That's cleaner and will avoid any other issues due to concurrent (but thread safe) 
updates of a variable.

As it stands now (the current code in trunk), a garbage string will be returned 
if the environment variable's value expands in size between calls. The second 
call to GetEnvironmentVariableA leaves the buffer untouched, and doesn't return 
0; instead it returns the required buffer size.

The logic would need to look something like this:

char dummybuf;
int size, size2;

SetLastError(0);
/*If the given bugger is not large enough to hold the data, the return value is 
the buffer size,  in characters, required to hold the string and its terminating 
null character. We use this return value to alloc the final buffer. */
size = GetEnvironmentVariableA(str, &dummybuf, 0);
if (size == 0) {
	if (GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
		/* The environment variable doesn't exist. */
		RETURN_FALSE;
	}

	/* env exists, but it is empty */
	RETURN_EMPTY_STRING();
}

ptr = emalloc(size);
SetLastError(0);
size2 = GetEnvironmentVariableA(str, ptr, size);
if (size == 0) {
	efree(ptr);

	if (GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
		/* The environment variable doesn't exist. */
		RETURN_FALSE;
	}

	/* env exists, but it is empty */
	RETURN_EMPTY_STRING();
} else if (size2 < size) {
	/* got the variable value */
	RETURN_STRING(ptr, 0);
} else {
	/* variable expanded between calls */
	/* ??? */
}

..where the /* ??? */ is replaced by however you propose to handle failure, I 
guess :)

Also, I've found that the non-Windows code path is actually also vulnerable to 
the same problem; running the same two scripts on Linux using Apache with the 
worker MPM and a threadsafe build of PHP produces the same results as Windows; 
the heap is leaked (and modifiable). (I guess why this is why the use of the 
worker MPM isn't recommended, huh)

Perhaps there is another issue in that the {get,put,unset}env() family of 
functions need to be used in a threadsafe manner. For example, use of getenv() 
would involve an invocation, and then its result being copied to a local buffer, 
atomically.

An alternative, if there are already the appropriate threadsafe data structures 
available, would be to make a local copy of the environment at process startup, 
and use that in the implementations of the PHP {get,put}env functions (and wherever else needed).

See my last comment, that covers what you are describing, if the length changed, 
we return FALSE and be done with it (as it can change over and over while we 
realloc the right buffer).

About unix and thread safe SAPI, it is well known that getenv is not thread safe 
on unix.