When making calls to apr_get_env on win32 platforms, if the given environment variable exists, but has a zero-length value, the apr_get_env call should return APR_SUCCESS with either an empty-string value or NULL. APR_ENOENT should only be returned if the requested environment variable does not exist in the environment. Enclosed (will attach it to this bug) is a patch which fixes this behavior.
Created attachment 19006 [details] patch to fix bug as described in report
Created attachment 19012 [details] updated patch to fix bug as described in report GetEnvironmentVariableW/A doesn't set the LastError to 0 on success. Therefore, in some cases [example, requesting an existing variable immediately following a request to a non-existant variable] we would incorrectly return ENOENT following a successful call to GetEnvironmentVariable. This updated patch explicitly resets the LastError value to 0 before calling GetEnvironmentVariable to avoid this issue.
Created attachment 19014 [details] testcase for this bug
You realize zero in this case means the the MS API isn't correctly requisitioning the space for a trailing NULL on the empty string? I was going to ask if we can avoid the test for NULL and special casing it, but apparently we would have to add the cycles to increment the returned size by 1 (with or without a test) so the special case looks as optimial as we can hope for. Did you have a chance to look at the unix behavior on any variety of unix platforms to see if there is at least a conformant subset? At least the test will let users verify proper behavior, thank you for that addition as well.
Applied in commit 471877 for apr 1.3.0/1.2.8/0.9.13, thank you for the patch. Had to massage your test case because TEST CASES MUST BE PLATFORM NEUTRAL :)
I don't have many UNIX boxes to play with it on... I frankly don't even have many different flavors of linux that I use these days (Debian, Ubuntu and Morphix - but that's all really just debian with different names). Regarding the test, I'm looking at the changes you made, and trying to figure out the reasoning (mainly so I can "do it right the first time" next time); I see you separated the check for get/set from the check for del. I don't understand why that makes it more platform neutral than checking for all of them together. I saw it as one subtest to check everything. We've already ascertained that get and/or set work as they should in previous tests; what are we accomplishing by separating the checks? Thanks for the feedback!
Platform neutrality was primarilly in the 'name' of the test. Although the error was flagged on win32, we are really testing that emptyenv works on any platform, not win32 specifically. That subset get/set could be invoked, so we did, and drop out before we get to 'delete'. We hadn't ascertained that empty strings could be set, before. Now we do. Your test goes on to exercise things in interesting ways which are useful, which is goodness, but unless you patch setenv to test for empty values, we don't want to lose an opportunity to test this scenario. In any case - thank you again for your submission and marking this done :)