{{ message }}
/ perl5 Public

# GetEnvironmentStrings() mess up the values for non-ascii strings#12161

Closed
opened this issue Jun 6, 2012 · 48 comments
Closed

# GetEnvironmentStrings() mess up the values for non-ascii strings#12161

opened this issue Jun 6, 2012 · 48 comments

to console Perl
"AAAбAAA"
to console Perl
"AAAÄAAA"
to console Perl
"AAAбAAA"
$VAR1 = "AAA\x{431}AAA"; perl ver parent v5.12.2 NEW PROCESS in ENVREAD.PL бл Active code page​: 65001 unicode=AAA?AAA perl ver reader v5.12.2 ### p5pRT commented Jun 13, 2012 ### From @bulk88 envread.pl ### p5pRT commented Jun 13, 2012 ### From @bulk88 Active code page​: 65001 SV = PV(0x9d820c) at 0xa39a14 REFCNT = 1 FLAGS = (PADMY,POK,pPOK,UTF8) PV = 0xa46574 "AAA\303\204AAA"\0 [UTF8 "AAA\x{c4}AAA"] CUR = 8 LEN = 12 env2.pl here бл Dump in Perl as Hex bytes "414141c384414141" to console bytes "AAAÃ�AAA"$VAR1 = "AAA\303\204AAA";
to console Perl
"AAAÄAAA"
\$VAR1 = "AAA\x{c4}AAA";

perl ver parent v5.12.2

NEW PROCESS

бл
Active code page​: 65001
unicode=AAAZAAA

### From @steve-m-hay

bulk88 via RT wrote on 2012-06-13​:

Okay, here is the problem with the patch, the env var is NOT
corrupted, only, if the env var is a utf8 char, that can be
represented in the current system high ascii CP. If the utf8 char can
not be represented in the current system high ascii CP, it is
converted to 0x3F. On pre- patch Perls, the previous case results in a
different letter, but not "?"
appearing in the child process.

Thanks for the sample programs. I intend to have a look through them when I have more time, but if I understand you correctly the problem here is only that things are still not working if the environment contains characters which cannot be represented in the current ANSI codepage?

That is to be expected, given the widechar (UTF-16) -> ANSI codepage conversion being done, and the ? character, of course, comes from the WideCharToMultiByte() calls when they fail to map a unicode character to the target encoding. On pre-patch perls something different happens because GetEnvironmentStringsA() was being used which returns things in the OEM codepage and must presumably have some algorithm of its own for handling unicode characters that cannot be mapped to that encoding, hence different characters (but still the wrong ones!) appearing instead of ?.

I still think the patch is worthwhile (with the fix I posted previously) because it fixes things for the common case where the environment contains characters which *are* in the current ANSI codepage but either not in the current OEM codepage (because the GetEnvironmentStringsA() conversion from unicode to OEM would have failed) or else (more commonly) in the OEM codepage but at a different byte position (because perl then treated the OEM bytes as if they were the ANSI bytes which are normally used).

So the only problem remaining is what happens in the case when characters outside of the current ANSI codepage appear in the environment, and that would be worth logging as a separate bug. It isn't such a common case, and there are likely to be problems in other areas of the code in that case anyway, e.g. you can't open a file whose name contains characters outside of the ANSI codepage without jumping through some hoops rather than using the built-in open() function.

If you agree and have no other issues with the patch then I'll apply it and raise the non-ANSI characters problem as a separate bug (and attach your scripts etc).

### From @bulk88

On Thu Jun 14 00​:56​:57 2012, Steve.Hay@​verosoftware.com wrote​:

So the
only problem remaining is what happens in the case when characters
outside of the current ANSI codepage appear in the environment, and
that would be worth logging as a separate bug. It isn't such a
common case, and there are likely to be problems in other areas of
the code in that case anyway, e.g. you can't open a file whose name
contains characters outside of the ANSI codepage without jumping
through some hoops rather than using the built-in open() function.
If you agree and have no other issues with the patch then I'll
apply it and raise the non-ANSI characters problem as a separate
bug (and attach your scripts etc).

I agree. You can apply the patch. Ultimately one day all of %INC support
in perlhost.h will have have to be made to use the wide apis so all wide
char env vars roundtrip correctly between processes and through %INC.
Note, to test this patch I used the raw wide win API since %INC was
unusable. Currently perlhost.h (which implements %INC) either queries
its own cache of env, or goes out to windows API.
http​://perl5.git.perl.org/perl.git/blob/3630f57ef8a29a646a6848f4e93d25ac47093a3c​:/win32/perlhost.h#l2402
http​://perl5.git.perl.org/perl.git/blob/3630f57ef8a29a646a6848f4e93d25ac47093a3c​:/win32/perlhost.h#l2084
http​://perl5.git.perl.org/perl.git/blob/3630f57ef8a29a646a6848f4e93d25ac47093a3c​:/win32/perlhost.h#l2134
http​://perl5.git.perl.org/perl.git/blob/3630f57ef8a29a646a6848f4e93d25ac47093a3c​:/win32/win32.c#l1653
Plus hv.c will need some fixing to add the utf8 flag to the %INC if the
env var isnt low ascii. Anyways, I'm done. You can apply the patch, and
there should be enough notes in this ticket for someone in the future to
work on the remainder of the problems.

### From karthik.rajagopalan@schrodinger.com

On Wed Jun 13 00​:22​:43 2012, shay wrote​:

On Tue Jun 12 08​:41​:15 2012, kartlee05 wrote​:

That was mistake on my part not to pass WC_NO_BEST_FIT_CHARS in
acutal
conversion call from UTF-16 to ACP. I will revise the patch and
sent
it
now.

BTW, I don't think the win32 code would really work if UTF-8 is
internally used for conversion. I see we use lot of calls toUpper,
tolower which would not really work with utf-8 directly. So it is
good
if ACP is used.

-Karthik

I now fixed the patch to use WC_NO_BEST_FIT_CHARS in the actual
conversion call. Please give a shot.

I think there is a bug in this and the previous patch (but not your
original one)​: the calculation of 'wenvstrings_len' wrongly sets
'size'
to 1 at the start of the for-loop, but never uses 'size' again. It
should have set 'wenvstrings_len' to 1 instead, otherwise that ends up
with the wrong value and causes many, many tests to fail when running
the full regression test suite.

After changing

for(size = 1; *lpWTmp != '\0'; lpWTmp += env_len + 1) {

to

for(wenvstrings_len = 1; *lpWTmp != '\0'; lpWTmp += env_len + 1) {

the patch looks good to me code-wise, but I will await any further
input
from bulk88 before committing.

I fixed the bug in my patch which failed to account for last '\0' char
in the environment strings.

Could you also fix up a couple of the comments, though, please? The
comment about converting from UTF-16 to UTF-8 should say *to the ANSI
codepage* instead. You also use C++ style comments in two places, but
perl always uses C style comments since most files are C and not all C

This is also fixed now.

The revised patch is attached now. Please take a look.

-Karthik

### From karthik.rajagopalan@schrodinger.com

From f5bbd57635ff725971d736aed43832a57845491b Mon Sep 17 00:00:00 2001
From: Karthik Rajagopalan <rajagopa@schrodinger.com>
Date: Thu, 14 Jun 2012 12:16:15 -0400
Subject: [PATCH] Use GetEnvironmentStringsW(..) instead of
GetEnvironmentStringsA(..).

GetEnvironmentStringsA(..) return strings in the OEM code page. This
can actually mangle the environment strings if it contain special characters.
A better approach would be to get the utf-16 strings through GetEnvironmentStringsW(..)
and convert them to ANSI code page. This is now done by win32_getenvironmentstrings(..).
To free the block, you can use win32_freeenvironmentstrings(..).
---
win32/perlhost.h |    8 ++++----
win32/win32.c    |   35 +++++++++++++++++++++++++++++++++++
win32/win32iop.h |    2 ++
3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/win32/perlhost.h b/win32/perlhost.h
index e8f5fb4..ae422ef 100644
--- a/win32/perlhost.h
+++ b/win32/perlhost.h
@@ -2262,7 +2262,7 @@ CPerlHost::CreateLocalEnvironmentStrings(VDir &vDir)
int nLength, compVal;

// get the process environment strings
-    lpAllocPtr = lpTmp = (LPSTR)GetEnvironmentStrings();
+    lpAllocPtr = lpTmp = (LPSTR)win32_getenvironmentstrings();

// step over current directory stuff
while(*lpTmp == '=')
@@ -2338,7 +2338,7 @@ CPerlHost::CreateLocalEnvironmentStrings(VDir &vDir)
}

// release the process environment strings
-    FreeEnvironmentStrings(lpAllocPtr);
+    win32_freeenvironmentstrings(lpAllocPtr);

return lpPtr;
}
@@ -2375,7 +2375,7 @@ CPerlHost::Clearenv(void)
}

/* get the process environment strings */
-    lpStr = lpEnvPtr = (LPSTR)GetEnvironmentStrings();
+    lpStr = lpEnvPtr = (LPSTR)win32_getenvironmentstrings();

/* step over current directory stuff */
while(*lpStr == '=')
@@ -2394,7 +2394,7 @@ CPerlHost::Clearenv(void)
lpStr += strlen(lpStr) + 1;
}

-    FreeEnvironmentStrings(lpEnvPtr);
+    win32_freeenvironmentstrings(lpEnvPtr);
}

diff --git a/win32/win32.c b/win32/win32.c
index 7f2444b..e496837 100644
--- a/win32/win32.c
+++ b/win32/win32.c
@@ -1650,6 +1650,41 @@ win32_ansipath(const WCHAR *widename)
}

DllExport char *
+win32_getenvironmentstrings(void)
+{
+    LPWSTR lpWStr, lpWTmp;
+    LPSTR lpStr, lpTmp;
+    DWORD env_len, wenvstrings_len = 0, aenvstrings_len = 0;
+
+    /* Get the process environment strings */
+    lpWTmp = lpWStr = (LPWSTR) GetEnvironmentStringsW();
+    for(wenvstrings_len = 1; *lpWTmp != '\0'; lpWTmp += env_len + 1) {
+        env_len = wcslen(lpWTmp);
+        /* calculate the size of the environment strings */
+        wenvstrings_len += env_len + 1;
+    }
+
+    /* Get the number of bytes required to store the UTF16 encoded string */
+    aenvstrings_len = WideCharToMultiByte(CP_ACP, WC_NO_BEST_FIT_CHARS,
+                                          lpWStr, wenvstrings_len, NULL, 0, 0, 0);
+    lpTmp = lpStr = (char *)win32_calloc(aenvstrings_len, sizeof(char));
+    if(!lpTmp)
+        out_of_memory();
+
+    /* Convert the string from UTF-16 encoding to ACP encoding */
+    WideCharToMultiByte(CP_ACP, WC_NO_BEST_FIT_CHARS, lpWStr, wenvstrings_len, lpStr,
+                        aenvstrings_len, 0, 0);
+
+    return(lpStr);
+}
+
+DllExport void
+win32_freeenvironmentstrings(void* block)
+{
+    win32_free(block);
+}
+
+DllExport char *
win32_getenv(const char *name)
{
dTHX;
diff --git a/win32/win32iop.h b/win32/win32iop.h
index 373e3e3..cbc9716 100644
--- a/win32/win32iop.h
+++ b/win32/win32iop.h
@@ -126,6 +126,8 @@ DllExport  void		win32_rewinddir(DIR *dirp);
DllExport  int		win32_closedir(DIR *dirp);
DllExport  DIR*		win32_dirp_dup(DIR *const dirp, CLONE_PARAMS *const param);

+DllExport  char*        win32_getenvironmentstrings(void);
+DllExport  void         win32_freeenvironmentstrings(void *block);
DllExport  char*	win32_getenv(const char *name);
DllExport  int		win32_putenv(const char *name);

--
1.7.7.1



### From karthik.rajagopalan@schrodinger.com

On Tue Jun 12 22​:54​:32 2012, bulk88. wrote​:

You can't use CP_ACP unless your intending for your patch to only work
with utf16 characters which are representable in the current system CP's
high ascii. Is this what you want?

Yes, there are lot of places in win32 which still depend on ACP calls. So
I just went ahead to do the same.

-Karthik

### From @steve-m-hay

On Thu Jun 14 09​:02​:31 2012, bulk88. wrote​:

On Thu Jun 14 00​:56​:57 2012, Steve.Hay@​verosoftware.com wrote​:

So the
only problem remaining is what happens in the case when
characters
outside of the current ANSI codepage appear in the environment,
and
that would be worth logging as a separate bug.

I agree. [...] Anyways, I'm done. You can apply the patch,
and
there should be enough notes in this ticket for someone in the future
to
work on the remainder of the problems.

I added a note to perltodo.pod in commit 799c141 referring to this
ticket since the problem of Unicode in the environment (and in filenames
etc) is already a known problem.

### From @steve-m-hay

On Thu Jun 14 09​:20​:33 2012, kartlee05 wrote​:

The revised patch is attached now. Please take a look.

Thanks, your patch is now applied as commit 4f46e52.

### p5pRT commented Jun 15, 2012

 @steve-m-hay - Status changed from 'open' to 'resolved'

closed this as completed Jun 15, 2012

### From karthik.rajagopalan@schrodinger.com

Hi Steve,

On Fri Jun 15 00​:57​:16 2012, shay wrote​:

On Thu Jun 14 09​:20​:33 2012, kartlee05 wrote​:

The revised patch is attached now. Please take a look.

Thanks, your patch is now applied as commit 4f46e52.

Thanks for applying this change. Will this be in 5.17? BTW, do you want
me to take a look and suggest changes that we would need to fully
support wide version for win32?

-Karthik

### From karthik.rajagopalan@schrodinger.com

On Fri Jun 15 07​:13​:34 2012, kartlee05 wrote​:

Hi Steve,

On Fri Jun 15 00​:57​:16 2012, shay wrote​:

On Thu Jun 14 09​:20​:33 2012, kartlee05 wrote​:

The revised patch is attached now. Please take a look.

Thanks, your patch is now applied as commit 4f46e52.

Thanks for applying this change. Will this be in 5.17?

I meant in next minor version for it?

BTW, do you want
me to take a look and suggest changes that we would need to fully
support wide version for win32?

-Karthik

### From [Unknown Contact. See original ticket]

On Fri Jun 15 07​:13​:34 2012, kartlee05 wrote​:

Hi Steve,

On Fri Jun 15 00​:57​:16 2012, shay wrote​:

On Thu Jun 14 09​:20​:33 2012, kartlee05 wrote​:

The revised patch is attached now. Please take a look.

Thanks, your patch is now applied as commit 4f46e52.

Thanks for applying this change. Will this be in 5.17?

I meant in next minor version for it?

BTW, do you want
me to take a look and suggest changes that we would need to fully
support wide version for win32?

-Karthik

### From karthik.rajagopalan@schrodinger.com

Sent from my iPhone

On Jun 13, 2012, at 3​:22 AM, "Steve Hay via RT" <perlbug-followup@​perl.org> wrote​:

On Tue Jun 12 08​:41​:15 2012, kartlee05 wrote​:

That was mistake on my part not to pass WC_NO_BEST_FIT_CHARS in acutal
conversion call from UTF-16 to ACP. I will revise the patch and sent
it
now.

BTW, I don't think the win32 code would really work if UTF-8 is
internally used for conversion. I see we use lot of calls toUpper,
tolower which would not really work with utf-8 directly. So it is good
if ACP is used.

-Karthik

I now fixed the patch to use WC_NO_BEST_FIT_CHARS in the actual
conversion call. Please give a shot.

I think there is a bug in this and the previous patch (but not your
original one)​: the calculation of 'wenvstrings_len' wrongly sets 'size'
to 1 at the start of the for-loop, but never uses 'size' again. It
should have set 'wenvstrings_len' to 1 instead, otherwise that ends up
with the wrong value and causes many, many tests to fail when running
the full regression test suite.

After changing

for(size = 1; *lpWTmp != '\0'; lpWTmp += env_len + 1) {

to

for(wenvstrings_len = 1; *lpWTmp != '\0'; lpWTmp += env_len + 1) {

I agree. I have not taken care for the additional null char to indicate the end of environment strings in my last two patches. I will add this now and resend.

the patch looks good to me code-wise, but I will await any further input
from bulk88 before committing.

Could you also fix up a couple of the comments, though, please? The
comment about converting from UTF-16 to UTF-8 should say *to the ANSI
codepage* instead. You also use C++ style comments in two places, but
perl always uses C style comments since most files are C and not all C

Sure, I will do that and send you in an hour.

-Karthik

### From @steve-m-hay

On Fri Jun 15 07​:13​:34 2012, kartlee05 wrote​:

Hi Steve,

On Fri Jun 15 00​:57​:16 2012, shay wrote​:

On Thu Jun 14 09​:20​:33 2012, kartlee05 wrote​:

The revised patch is attached now. Please take a look.

Thanks, your patch is now applied as commit 4f46e52.

Thanks for applying this change. Will this be in 5.17? BTW, do you want
me to take a look and suggest changes that we would need to fully
support wide version for win32?

The change will indeed be in 5.17.1, due out in a couple of days.

Support for wide api calls on Win32 was actually removed some time ago
(5.8.1) since it was not working correctly and it was deemed, IIRC, that
it would be more worthwhile to address the wider issue of supporting
Unicode in any filesystems that support it rather than fixing the broken
support for just Windows NTFS, hence the items in Porting/todo.pod about
"Unicode in Filenames", "Unicode in %ENV" and "Unicode and glob()".

For sure, the solution on Windows must involve the wide apis, but what
we really need first is a framework for hooking those calls into as
described in the item "Virtualize operating system access" in
Porting/todo.pod.

### From @bulk88

On Mon Jun 18 00​:26​:30 2012, shay wrote​:

The change will indeed be in 5.17.1, due out in a couple of days.

This ticket probably caused the leak reported at https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121676

--
bulk88 ~ bulk88 at hotmail.com

mentioned this issue Feb 12, 2020