463326 - WinCE missing ExpandEnvironmentStringsW function

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[John Wolfe (:wolfe)]

Attachment: v1.0 patch

Need to implement a ExpandEnvironmentStringsW function for WinCE Shunt Library.

Comment 1

[John Wolfe (:wolfe)]

Attachment: v2.0 patch

Changed DWORDs in ExpandEnvironmentSettingsW function prototype to "unsigned int"

Added safety check for lpDst == NULL

Changed _T('%') to L'%' in two places

Comment 2

[John Wolfe (:wolfe)]

Attachment: Test Program for new function ExpandEnvironmentStringsW

This test program runs the first of each string pair through the ExpandEnvironmentStringsW function, and compares to the second of each string pair.  This test runs successfully for all pairs, producing this output in the Visual Studio 2008 Output window:

Load module: shunt_test.exe
Load module: mozce_shunt.dll
Load module: coredll.dll.0409.MUI
Load module: coredll.dll
Expand Test # 0 SUCCESS
Expand Test # 1 SUCCESS
Expand Test # 2 SUCCESS
Expand Test # 3 SUCCESS
Expand Test # 4 SUCCESS
Expand Test # 5 SUCCESS
Expand Test # 6 SUCCESS
Expand Test # 7 SUCCESS
Expand Test # 8 SUCCESS
Expand Test # 9 SUCCESS
Unload module: mozce_shunt.dll
The program '[0xDED704EE] shunt_test.exe' has exited with code 0 (0x0).

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 346580 [details] [diff] [review]
v2.0 patch


+      if ( origLen < index + envlen ) {    // Ran past end of original
+        SetLastError(ERROR_INVALID_PARAMETER); // buffer without matching '%'
+        return -1;
+      }
+    }

This shouldn't be an error.  According to MSDN, "f the name is not found, the %variableName% portion is left unexpanded," so we should just copy it over unexpanded and move on

Nothing else jumps out at me, so if you account for this case we should be good to go.

Comment 4

[John Wolfe (:wolfe)]

Sorry - the block of code referenced in the last comment actually deals with when trying to figure out the string in between two '%' characters, if we run out of input buffer, then there is a bogus mis-matched single '%' - and that throws an error.

What Brad is talking about is covered by this code:

+      if ( NULL != pC ) {
+        int n = MultiByteToWideChar( CP_ACP, 0, pC, -1, pOut, nSize - size );
+        if ( n > 0 ) {
+          size += n - 1;  // Account for trailing zero
+          pOut += n - 1;
+        }
+      }

Where pC should be the result of calling getenv() with the provided environment variable properly nestled within two '%' characters.  If getenv() returns NULL -- meaning that the environment variable is not found, then we just skip by the not-found environment variable, just as the MSDN docs declare we should.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

two other things

the signature should be (const unsigned short*, unsigned short*, unsigned int) to match the windows api


The second is more of a question. What is the correct behavior if the replacement contains a variable? for instance PATH=%GREPATH%;%TOOLS_PATH% and I call expand env vars with "My path is %PATH%".  Should we recursively call expand env vars on the expansions?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #4)
> Sorry - the block of code referenced in the last comment actually deals with
> when trying to figure out the string in between two '%' characters, if we run
> out of input buffer, then there is a bogus mis-matched single '%' - and that
> throws an error.

Yup, I misread that.

Comment 7

[John Wolfe (:wolfe)]

Attachment: v3.0 Patch

Changes first argument to ExpandEnvironmentSettings() to be const unsigned short *

Changes behavior if ENV VAR not found - in this case, the %VariableName% is just copied to the output.  This also means that "%%" appears on the output string as "%%".

On WinXP, ExpandEnvironmentSettings() function does not do recursive expansion of environment variables.  HOWEVER, on WinXP, if you set an environment variable like this:

MyEnvVar="The path is [%PATH%]"

Then MyEnvVar is set to be:

The path is [WHATEVER_THE_PATH_IS_WHEN_MyEnvVar_IS_SET]

So there is an evaluation/expansion of %VarName% when environment variables are set -- which we are NOT doing yet in our shunt.

NOTE: the current code does not do case-insensitive comparisons.  This may be a problem needing solving in the future.  

ALSO: The intention of this function call seems to be to expand things like %ProgramFiles% or %SystemRoot% - neither of which is in our environment variable list right now.  When we get a running build, I will add some output to identify missing environment variables - so we can back-fill into our initial environment variable listing.

Comment 8

[John Wolfe (:wolfe)]

Attachment: v2.0 of testing program for ExpandEnvironmentSettings function

This test program was modified based upon results of running similar tests on a Windows XP system.

The behavior of ExpandEnvironmentSettings() was different from expected from reading the MSDN documentation on the function.

Comment 9

[John Wolfe (:wolfe)]

Attachment: v1.0 of Simple WinXP Test program

v1.0 of Simple WinXP Test program for ExpandEnvironmentSettings() function

Includes comments about relevant environment variables set under my Windows XP machine, as well as output from running the test program.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

First, apologies for taking so long to review.

(In reply to comment #7)
> ALSO: The intention of this function call seems to be to expand things like
> %ProgramFiles% or %SystemRoot% - neither of which is in our environment
> variable list right now.  When we get a running build, I will add some output
> to identify missing environment variables - so we can back-fill into our
> initial environment variable listing.

Can we back the missing variables with SHGetSpecialFolderPath?

Comment 11

[John Wolfe (:wolfe)]

Attachment: v4.0 patch - adds several special folder env settings

v4.0 patch - adds some special folders to environment variables on DLL load

Adds several special folders into environment variables, using the same
environment variable names as the desktop.  Here are the three currently added:

APPDATA = "\Application Data"
ProgramFiles = "\Program Files"
windir = "\Windows"

Comment 12

[Doug Turner (:dougt)]

changeset:   22817:b7361703cb40
tag:         tip
user:        John Wolfe <[e-mail]>
date:        Mon Dec 15 10:16:07 2008 -0800
summary:     Bug 463326 - WinCE missing ExpandEnvironmentStringsW function. r=dougt, blassey

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/03ec8e3a97a6