bug#2062: PATH can contain non-expanded variables

bug#2062: PATH can contain non-expanded variables

[Juanma Barranquero]

Package: emacs,w32
Version: 23.0.60

It is possible for PATH (or other path-like variables, like EMACSPATH
and EMACSLOADPATH) to contain unexpanded variables:

ELISP> exec-path
("c:/emacs/bin" "%MYTEST%" "C:/WINDOWS/system32" "C:/WINDOWS"
"C:/WINDOWS/system32/Wbem" "c:/emacs/trunk/bin")

for example, if the user has set
"HKLM\Microsoft\Windows\CurrentVersion\App Paths\emacs.exe\Path" as a
REG_EXPAND_SZ.

These variables should be expanded, but currently
emacs.c:decode_env_path does not. The following patch fixes it.

Comments?

    Juanma


Index: emacs.c
===================================================================
RCS file: /sources/emacs/emacs/src/emacs.c,v
retrieving revision 1.461
diff -u -2 -r1.461 emacs.c
--- emacs.c	23 Jan 2009 14:53:11 -0000	1.461
+++ emacs.c	26 Jan 2009 12:48:49 -0000
@@ -2467,6 +2467,12 @@
   if (path)
     {
+#ifdef WINDOWSNT
+      DWORD required = ExpandEnvironmentStrings (path, NULL, 0);
+      p = (char *) alloca (required);
+      ExpandEnvironmentStrings (path, p, required);
+#else
       p = alloca (strlen (path) + 1);
       strcpy (p, path);
+#endif
       path = p;

bug#2062: PATH can contain non-expanded variables

[Lennart Borgman]

On Mon, Jan 26, 2009 at 2:00 PM, Juanma Barranquero <lekktu@gmail.com> wrote:
> Package: emacs,w32
> Version: 23.0.60
>
> It is possible for PATH (or other path-like variables, like EMACSPATH
> and EMACSLOADPATH) to contain unexpanded variables:
>
> ELISP> exec-path
> ("c:/emacs/bin" "%MYTEST%" "C:/WINDOWS/system32" "C:/WINDOWS"
> "C:/WINDOWS/system32/Wbem" "c:/emacs/trunk/bin")
>
> for example, if the user has set
> "HKLM\Microsoft\Windows\CurrentVersion\App Paths\emacs.exe\Path" as a
> REG_EXPAND_SZ.
>
> These variables should be expanded, but currently
> emacs.c:decode_env_path does not. The following patch fixes it.
>
> Comments?

Nice. Thanks.

bug#2062: PATH can contain non-expanded variables

[Jason Rumney]

Juanma Barranquero wrote:
> Index: emacs.c
> ===================================================================
> RCS file: /sources/emacs/emacs/src/emacs.c,v
> retrieving revision 1.461
> diff -u -2 -r1.461 emacs.c
> --- emacs.c	23 Jan 2009 14:53:11 -0000	1.461
> +++ emacs.c	26 Jan 2009 12:48:49 -0000
> @@ -2467,6 +2467,12 @@
>    if (path)
>      {
> +#ifdef WINDOWSNT
> +      DWORD required = ExpandEnvironmentStrings (path, NULL, 0);
> +      p = (char *) alloca (required);
> +      ExpandEnvironmentStrings (path, p, required);
> +#else
>        p = alloca (strlen (path) + 1);
>        strcpy (p, path);
> +#endif
>        path = p;
>   

The right place to do this expansion is where the variable is read from 
the registry, and only when the type is REG_EXPAND_SZ.

bug#2062: PATH can contain non-expanded variables

[Juanma Barranquero]

On Tue, Jan 27, 2009 at 02:56, Jason Rumney <jasonr@f2s.com> wrote:

> The right place to do this expansion is where the variable is read from the
> registry, and only when the type is REG_EXPAND_SZ.

The problem is, we don't read
"HKLM\Software\Microsoft\Windows\CurrentVersion\App
Paths\emacs.exe\Path", we find the PATH made for us and it contains
unexpanded variables.

My patch modifies decode_env_path, because it is used for PATH-like
variables, not generic environment variables. How likely is that such
a PATH-like variable contains a string like %VARIABLENAME%, meaning a
literal "%VARIABLENAME%" directory or filename (or part of it)? That's
a hypothetical problem, while my path containing such a variable is a
fact.

    Juanma

bug#2062: PATH can contain non-expanded variables

[Jason Rumney]

Juanma Barranquero wrote:
> On Tue, Jan 27, 2009 at 02:56, Jason Rumney <jasonr@f2s.com> wrote:
>
>   
>> The right place to do this expansion is where the variable is read from the
>> registry, and only when the type is REG_EXPAND_SZ.
>>     
>
> The problem is, we don't read
> "HKLM\Software\Microsoft\Windows\CurrentVersion\App
> Paths\emacs.exe\Path", we find the PATH made for us and it contains
> unexpanded variables.
>   

If the system does not handle REG_EXPAND_SZ in the App Paths registry 
values, then the bug is with whatever incorrectly used that type when 
setting the registry value.

bug#2062: PATH can contain non-expanded variables

[Juanma Barranquero]

On Tue, Jan 27, 2009 at 10:24, Jason Rumney <jasonr@f2s.com> wrote:

> If the system does not handle REG_EXPAND_SZ in the App Paths registry
> values, then the bug is with whatever incorrectly used that type when
> setting the registry value.

I'd say that Microsoft is of two minds about it, judging by the
entries for msimn.exe (Outlook Express), pbrush.exe (mspaint) and a
few others, that do use REG_EXPAND_SZ.

That said, ExpandEnvironmentStrings is not *specifically* for
REG_EXPAND_SZ registry entries; it is generally useful for cases such
as this one.

So we can either:

  a) consider it as a bug outsize Emacs, and do nothing, or

  b) decide that a variable-like string of the form %VARIABLE% in a
PATH-like value is much, much likely to be that, an unexpanded env
var, than part of a file or directory name, and so expand it.

I'm obviously in camp b), and would like to commit this patch. If
you're strongly on camp a, though, I'll close this bug as notabug +
wontfix.

    Juanma

bug#2062: PATH can contain non-expanded variables

[Jason Rumney]

Juanma Barranquero wrote:
>   b) decide that a variable-like string of the form %VARIABLE% in a
> PATH-like value is much, much likely to be that, an unexpanded env
> var, than part of a file or directory name, and so expand it.
>   

If we decide b, then why would we only decide that for Windows (syntax 
excepted)?

bug#2062: PATH can contain non-expanded variables

[Juanma Barranquero]

On Tue, Jan 27, 2009 at 14:29, Jason Rumney <jasonr@gnu.org> wrote:

> If we decide b, then why would we only decide that for Windows (syntax
> excepted)?

No particular reason, except that REG_EXPAND_SZ exists, and so, having
variables in PATH is not uncommon on Windows, while it is perhaps
uncommon in other OSes. But that's no reason not to generalize it, if
deemed useful, of course.

    Juanma

bug#2062: PATH can contain non-expanded variables

[Stefan Monnier]

>> b) decide that a variable-like string of the form %VARIABLE% in a
>> PATH-like value is much, much likely to be that, an unexpanded env
>> var, than part of a file or directory name, and so expand it.
> If we decide b, then why would we only decide that for Windows (syntax
> excepted)?

It looks like a situation where the variable expansion is expected to be
done by the application under w32 (just like file globbing is not done
by the shell there either), contrary to POSIX.
So it's definitely not needed in GNU/Linux, and probably not in Mac OS
X either.  I cannot judge whether Juanma's patch is doing the right
thing, or is working around some bug/misfeature elsewhere, or is really
doing something wrong (it looks like the second option, tho).


        Stefan

bug#2062: PATH can contain non-expanded variables

[Juanma Barranquero]

On Tue, Jan 27, 2009 at 20:02, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> I cannot judge whether Juanma's patch is doing the right
> thing, or is working around some bug/misfeature elsewhere, or is really
> doing something wrong (it looks like the second option, tho).

I think my patch works around a underspecified part of PATH
processing. In my quite-informal tests, it seems like App Path
expansion is done, or not done, differently for different command
shells, and also depends on whether you run the program from CMD or
the Windows Explorer shell.

    Juanma

bug#2062: PATH can contain non-expanded variables

[Jason Rumney]

Stefan Monnier wrote:
> It looks like a situation where the variable expansion is expected to be
> done by the application under w32 (just like file globbing is not done
> by the shell there either), contrary to POSIX.
>   
Environment variables are expanded in PATH when set as an environment 
variable on Windows. Only when using this undocumented registry entry to 
set an application specific path does the expansion apparently not happen.

bug#2062: PATH can contain non-expanded variables

[Juanma Barranquero]

On Wed, Jan 28, 2009 at 09:39, Jason Rumney <jasonr@gnu.org> wrote:

> Only when using this undocumented registry entry [...]

Underdocumented, or badly documented, perhaps. Undocumented is not:

http://msdn.microsoft.com/en-us/library/cc144150(VS.85).aspx

    Juanma

bug#2062: PATH can contain non-expanded variables

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Cc: 2062@emacsbugs.donarmstrong.com
> 
> On Tue, Jan 27, 2009 at 10:24, Jason Rumney <jasonr@f2s.com> wrote:
> 
> > If the system does not handle REG_EXPAND_SZ in the App Paths registry
> > values, then the bug is with whatever incorrectly used that type when
> > setting the registry value.
> 
> I'd say that Microsoft is of two minds about it, judging by the
> entries for msimn.exe (Outlook Express), pbrush.exe (mspaint) and a
> few others, that do use REG_EXPAND_SZ.
> 
> That said, ExpandEnvironmentStrings is not *specifically* for
> REG_EXPAND_SZ registry entries; it is generally useful for cases such
> as this one.

What was the use-case that caused Path to include unexpanded
environment variables in your case?

bug#2062: PATH can contain non-expanded variables

[Juanma Barranquero]

On Tue, Jan 27, 2009 at 20:30, Eli Zaretskii <eliz@gnu.org> wrote:

> What was the use-case that caused Path to include unexpanded
> environment variables in your case?

In *my* case, I run several instances of Emacs, and I put different
binaries (image libraries, etc.) in several directories, pointed to by
one environment variable. I always start Emacs from the command line,
and I set the environment variable according to which Emacs I want to
run and what additional binaries to use. I have the App Path set to
C:\emacs\%myvar%\bin, where %myvar% points to the relevant dir.

But my use case is not important: there are many ways to do what I do,
and I'm not even claiming mine is optimal, or close to it.

If I want to fix that (or work around it, if it is a Windows bug) is
because certainly there's nothing precluding the user from setting a
REG_EXPAND_SZ for PATH, or EMACSPATH or EMACSLOADPATH, and there seems
to be no downside to this change, unless we deem
C:\this\is%myweird%path\to\file likely.

But this does not merit too long a discussion IMO. Either you (all)
think it's useful, or hackish. Your call.

    Juanma

bug#2062: PATH can contain non-expanded variables

[Eli Zaretskii]

> Date: Tue, 27 Jan 2009 20:55:09 +0100
> From: Juanma Barranquero <lekktu@gmail.com>
> Cc: 2062@emacsbugs.donarmstrong.com, jasonr@f2s.com
> 
> But this does not merit too long a discussion IMO. Either you (all)
> think it's useful, or hackish. Your call.

I was trying to understand how come a variable is stored in the
Registry with incorrect REG_EXPAND_SZ, and how often that happens.  Is
that true that in your case it is simply a user mistake?

bug#2062: PATH can contain non-expanded variables

[Juanma Barranquero]

On Wed, Jan 28, 2009 at 05:12, Eli Zaretskii <eliz@gnu.org> wrote:

> I was trying to understand how come a variable is stored in the
> Registry with incorrect REG_EXPAND_SZ, and how often that happens.  Is
> that true that in your case it is simply a user mistake?

Of course not. It was done on purpose, if that's what you're asking.
But I don't see anything that suggests that REG_EXPAND_SZ is not valid
for App Path entries. In fact, Microsoft adds such entries, and
perusing the MSDN seems to indicate that they are indeed valid.

Not only that: if I start Emacs from a desktop shortcut (and, thus,
through ShellExecute), App Paths is correctly expanded.

    Juanma

bug#2062: PATH can contain non-expanded variables

[Stefan Monnier]

> Not only that: if I start Emacs from a desktop shortcut (and, thus,
> through ShellExecute), App Paths is correctly expanded.

So your patch works around a bug in some other place which fails to
expand PATH.


        Stefan

bug#2062: marked as done (PATH can contain non-expanded variables)

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]


Your message dated Wed, 28 Jan 2009 16:07:03 +0100
with message-id <f7ccd24b0901280707u39a4364bk76339f2e5956fa5@mail.gmail.com>
and subject line Re: bug#2062: PATH can contain non-expanded variables
has caused the Emacs bug report #2062,
regarding PATH can contain non-expanded variables
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
2062: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=2062
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 3265 bytes --]

From: Juanma Barranquero <lekktu@gmail.com>
To: Emacs Bug Tracker <submit@emacsbugs.donarmstrong.com>
Subject: PATH can contain non-expanded variables
Date: Mon, 26 Jan 2009 14:00:15 +0100
Message-ID: <f7ccd24b0901260500h18a7ac63uf5c88cbb515ddeb0@mail.gmail.com>

Package: emacs,w32
Version: 23.0.60

It is possible for PATH (or other path-like variables, like EMACSPATH
and EMACSLOADPATH) to contain unexpanded variables:

ELISP> exec-path
("c:/emacs/bin" "%MYTEST%" "C:/WINDOWS/system32" "C:/WINDOWS"
"C:/WINDOWS/system32/Wbem" "c:/emacs/trunk/bin")

for example, if the user has set
"HKLM\Microsoft\Windows\CurrentVersion\App Paths\emacs.exe\Path" as a
REG_EXPAND_SZ.

These variables should be expanded, but currently
emacs.c:decode_env_path does not. The following patch fixes it.

Comments?

    Juanma


Index: emacs.c
===================================================================
RCS file: /sources/emacs/emacs/src/emacs.c,v
retrieving revision 1.461
diff -u -2 -r1.461 emacs.c
--- emacs.c	23 Jan 2009 14:53:11 -0000	1.461
+++ emacs.c	26 Jan 2009 12:48:49 -0000
@@ -2467,6 +2467,12 @@
   if (path)
     {
+#ifdef WINDOWSNT
+      DWORD required = ExpandEnvironmentStrings (path, NULL, 0);
+      p = (char *) alloca (required);
+      ExpandEnvironmentStrings (path, p, required);
+#else
       p = alloca (strlen (path) + 1);
       strcpy (p, path);
+#endif
       path = p;



[-- Attachment #3: Type: message/rfc822, Size: 3903 bytes --]

From: Juanma Barranquero <lekktu@gmail.com>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: 2062-close@emacsbugs.donarmstrong.com, Eli Zaretskii <eliz@gnu.org>, jasonr@f2s.com
Subject: Re: bug#2062: PATH can contain non-expanded variables
Date: Wed, 28 Jan 2009 16:07:03 +0100
Message-ID: <f7ccd24b0901280707u39a4364bk76339f2e5956fa5@mail.gmail.com>

On Wed, Jan 28, 2009 at 15:24, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> So your patch works around a bug in some other place which fails to
> expand PATH.

Not only you're right, but I know which program is causing the problem. Thanks!

According to MSDN KB 837633 (and other sources):

  Windows XP adds the path value, if it exists, to the PATH
environment variable,
  if you use the ShellExecute() function to start your program.

So, correctly, Windows Explorer uses App Paths, and CMD.EXE does not
(I've checked it).

The problem is with TCC (formerly 4NT), a proprietary command shell I
use. I had mistakenly assumed that TCC worked like CMD (or, in fact, I
did the reverse: because TCC adds App Paths to PATH, I assumed that
CMD also did it).

But it is not: apparently, TCC is adding App Paths to PATH (which is
either a bug or a feature, depending of one's POV), but it's not
taking into account that the App Paths entry can be REG_EXPAND_SZ (and
that's definitely a bug).

I'll report it to them.

    Juanma