Summary: | apr_get_env calls with zero-length values should not return APR_ENOENT on Windows | ||
---|---|---|---|
Product: | APR | Reporter: | Issac Goldstand <margol> |
Component: | APR | Assignee: | Apache Portable Runtime bugs mailinglist <bugs> |
Status: | RESOLVED FIXED | ||
Severity: | normal | Keywords: | PatchAvailable |
Priority: | P2 | ||
Version: | HEAD | ||
Target Milestone: | --- | ||
Hardware: | PC | ||
OS: | Windows XP | ||
Attachments: |
patch to fix bug as described in report
updated patch to fix bug as described in report testcase for this bug |
Description
Issac Goldstand
2006-10-16 06:23:09 UTC
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 :) |