129401 - External execution on Win32/AS Perl requires CMD.EXE be in your PATH

Status: RESOLVED WORKSFORME

(Bugzilla :: Bugzilla-General, defect)

Description

[Cédric Caron]

Under windows 2000 the system function is not working properly if the PATH 
environment variable have been deleted.

removing PATH from the deleted variables (in global.pl) fix the problem

Comment 1

[Dave Miller [:justdave]]

which system function?  There are a lot of them, and PATH should be irrelevant
if a full pathname is supplied.

I'm not going to put PATH back, because it breaks taint mode if it's present. 
So we need to find a way to work around it.

Can you provide us an error message, or a code snippet showing which one failed?

Comment 2

[Cédric Caron]

Under windows 2000/NT system is using the shell (cmd.exe) to start the 
requested command. If you delete the PATH variable system failed with the 
folowing error message "Can't spawn "cmd.exe": No such file or directory at 
C:\DL\Devlopment\BugZilla\current\mozilla\webtools\bugzilla\process_bug.cgi 
line 1361*

Comment 3

[Cédric Caron]

I solved the problem by adding the folowing code in global.pl

RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v
retrieving revision 1.147
diff -r1.147 globals.pl
83a84,86
> if($^O =~ /win32/i) {
> 	$::ENV{'PATH'}=$::ENV{'SystemRoot'}."\\system32";
> }

Comment 4

[Dave Miller [:justdave]]

ya know, I'd be tempted to call that a bug in Perl...  :-)

But as long as we know about it we might as well work around it.

Explicitly setting the PATH gets around the taint checks.  Although I'm not sure
whether $ENV{SystemRoot} will be considered tainted or not, so we may have the
same problem since you're basing the PATH on SystemRoot.

bbaetz: since you're our taint mode expert lately :-)  would you happen to be
able to throw together a quick test script for him to run that does a check on
$::ENV{System_Root} to see if it's tainted or not when the script starts?

Comment 5

[Jacob Steenhagen]

Attachment: test.pl -- taint test case

The script is really simple... it uses is_tainted from globals.pl (borrowed
from elsewhere) to determine if the variable is tainted or not.

C:\Mozilla>perl -T test.pl
SystemRoot is tainted

If you uncomment the line that explicitly sets $ENV{SystemRoot}, then it says
it's safe.

Comment 6

[Dave Miller [:justdave]]

OK, so SystemRoot has the same problem PATH does then.  How do we work around
this?  Do we check for known safe locations for SystemRoot or do we just blindly
untaint it and hope they have their system set up right? :-)

Comment 7

[Cédric Caron]

Why not adding a systempath variable to localconfig and setting the path to 
this value ?

during the setup checksetup can copy the content of SystemRoot in the variable 

r you ok with a non taintsafe checksetup ?

Comment 8

[Dave Miller [:justdave]]

That would probably work.  Presumably the operator is in control of his
environment when running checksetup.pl, so we consider that safe.  We do that
already to get the path to mysql in checksetup.pl.

Comment 9

[Cédric Caron]

Attachment: Patch to add a systempath variable in localconfig and set PATH to systempath

I modified checksetup to create a variable caled systempath and set it to the
corect value and modified global.pl to set PATH to this value

Comment 10

[Cédric Caron]

Attachment: Patch to add a systempath variable in localconfig and set PATH to systempath

I modified checksetup to create a variable caled systempath and set it to the
corect value and modified global.pl to set PATH to this value

Comment 11

[Dave Miller [:justdave]]

Comment on attachment 73011 [details] [diff] [review]
Patch to add a systempath variable in localconfig and set PATH to systempath

marking duplicate patch obsolete

Comment 12

[Bradley Baetz (:bbaetz)]

Taint mode probably isn't enabled for win32, because the #! lines arne't parsed.
Is that right?

Comment 13

[Bradley Baetz (:bbaetz)]

Also see bug 129543 - is that related to this? how is ./ working for you?

Comment 14

[Dave Miller [:justdave]]

I believe Perl parses the #! line itself looking for flags if it isn't given any
flags on the command line when it starts.

I know if I have

#!/usr/bin/perl -wT

on the #! line and I do "perl -wc filename" it tells me "too late for -T at line 1"

Dunno how win32 handles it

Comment 15

[Cédric Caron]

to answer to the question "how is ./ working for you?" than answer is : I 
reported the bug 129543 ;-)

Comment 16

[Cédric Caron]

Under windows the #! line iy not parsed by the system but is parsed by perl.

Making perl under windows work exactly like perl under unix (on this point...).

Comment 17

[Cédric Caron]

I reported this bug in the system function to active state 
(http://bugs.activestate.com/show_bug.cgi?id=19287&submit=Go)

Comment 18

[Bradley Baetz (:bbaetz)]

What about teh missing extention for calling processmail? ISTR someone
mentioning that somewhere.

Can't you use $^X ? On unix it shuld be the path from the #! line, which is
absolute, and the wnidows file associateion stuff should make it absolute too -
can someone print $^X from a script with a .pl extention run from a web server
on windows?
How did this ever work on windows (the ./ stuff, I mean)?

Comment 19

[Cédric Caron]

the ./ stuf is not working under windows. To make it working you need to 
write .\

Comment 20

[Bradley Baetz (:bbaetz)]

Right, and $^X is tainted. Maybe we should just untaint it?

But how did this work pre-2.14?

Comment 21

[Cédric Caron]

it is not working in 2.14 you have to do the same modification...

Comment 22

[Matthew Tuck [:CodeMachine]]

It never worked I believe, this is documented in the Win32 install instructinos.

Comment 23

[Bradley Baetz (:bbaetz)]

'processmail should be a sub' ;)

Comment 24

[Zach Lipton [:zach]]

*** Bug 130156 has been marked as a duplicate of this bug. ***

Comment 25

[Zach Lipton [:zach]]

*** Bug 130171 has been marked as a duplicate of this bug. ***

Comment 26

[Dave Miller [:justdave]]

Since the original bug filed at ActiveState seemed to be a little light on
information, I went and tried to add some more information to it. 
Unfortunately, ActiveState doesn't seem to allow anyone other than the reporter
and the assignee (and probably people with editbugs privs) to add comments, so I
filed a new bug to give them more information on it.

http://bugs.activestate.com/show_bug.cgi?id=19369

Comment 27

[Bradley Baetz (:bbaetz)]

-> 2.18. win32 is not a showstopper for 2.16, and it isn't clear that this is
our bug anyway.

Comment 28

[Jouni Heikniemi]

Comment on attachment 73012 [details] [diff] [review]
Patch to add a systempath variable in localconfig and set PATH to systempath

This patch is no longer applicable as it's based on more than 
20 revisions old files and is not a unidiff, so the line 
offset is wrong. Please regenerate the patch as a diff -u 
against the current trunk codebase. 

Some nits, then:

>> my $systempath = "$::ENV{'SystemRoot'}\\system32";
>> $systempath =~ s/\\/\\\\/g;
>> LocalVar('systempath', <<"END");

These lines inside the if block should have a four space indentation. 

Do you know if SystemRoot is guaranteed not to end in a slash? 
Though, Windows runs c:\winnt\\system32\cmd.exe without problems,
so maybe that isn't critical anyway.

>> 	$::ENV{'PATH'}=$::systempath;

There is a tab on this line - it should be four spaces of course. 
You could also add spaces around the assignment operator.

I'll test this on my systems when a working patch is ready.

Comment 29

[Cédric Caron]

Attachment: patch v2

Updated patch to work with current CVS version

Comment 30

[Dave Miller [:justdave]]

Date: 16 Dec 2002 20:05:19 -0000
From: bugzilla <[e-mail]>
To: [e-mail]
Subject: [ActivePerl Bug 19369] system() fails with "can't find CMD.EXE" if PATH
has been changed/deleted
X-Bugzilla: http://bugs.activestate.com/


http://bugs.activestate.com/show_bug.cgi?id=19369
[e-mail] changed:

           What    |Old Value                   |New Value
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
           Platform|                            |Any
         Resolution|                            |Not a Bug



------- Comments from [e-mail]  2002-12-16 12:05
I don't consider this a bug.  Perl can't know the absolute location
of cmd.exe without relying on Win32 "shell" interfaces it doesn't
want to rely on without an extremely good reason.  And besides,
one could argue the other way and say Perl should pay attention to
PATH when picking up its default command shell.

If you're clobbering PATH, you should then set the PERL5SHELL
variable in the environment to use the absolute location of
cmd.exe so that perl doesn't have to rely on PATH to look it
up.  See perlrun.pod for exactly what to set it to.

Comment 31

[Cédric Caron]

This need to be documented in the win32 instalation guide !!!

Comment 32

[Marc Bauer]

please use only "/" in path on win32... this is perl standard. perl will 
convert this in background to "\\" - if perl runs on win32...

Comment 33

[Jason Pyeron]

This works for me
Microsoft Windows 2000 [Version 5.00.2195]
Bugzilla 2.17.4
Perl v5.8.0 built for cygwin-multi-64int

I need more info on what is happening, can i get a Buzilla version for comment 2

here is what I tried to reproduce the claims:

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>set PATH
Path=C:\WINNT\system32;C:\WINNT;C:\WINNT\System32\Wbem;C:\Program Files\Common 
Files\Adaptec Shared\System;c:\cygwin\bin
;C:\cygwin\usr\X11R6\bin;C:\j2sdk1.4.1_01\bin;c:\geda\bin;C:\Program 
Files\Support Tools\;C:\Program Files\Common Files\
Autodesk Shared\;C:\Program Files\Microsoft SQL Server\80
\Tools\Binn\;C:\Program Files\jakarta-ant-1.5.1\bin;C:\Program
Files\Rational\ClearQuest;C:\BC5\BIN;


C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>cat test.pl
#!/usr/bin/perl

print "trying\n";
print system("test2.pl"), "\n";
print "done\n";

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>cat test2.pl
#!/usr/bin/perl

print "test2.pl\n";

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>test.pl
trying
test2.pl
0
done

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>set PATH=

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>set PATH

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>test.pl
[popup asking for perl.exe location]
Access is denied.

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>set PATH=c:\cygwin\bin

C:\usr\jpyeron\proj\jBugzilla\Bugzilla\tip>test.pl
trying
test2.pl
0
done

Comment 34

[Dave Miller [:justdave]]

Jason: I believe the problem is actually with ActiveState Perl.  I'd be willing
to bet Cygwin Perl "Does The Right Thing" since it's based more closely on the
unix Perl. :)

Comment 35

[Jason Pyeron]

given ActiveState's perl is 'iffy' can ActiveState and Cygwin be added to the 
OS list?

Comment 36

[Marc Bauer]

hi guys

i'd like to explain you how cmd.exe work - it looks like there are some
missunderstandings. you are trying to set a win environment var with cmd.exe and
SET - this isn't possible!!! if you do a "SET PATH=C:\Test" the environment var
isn't set to the machine environment. the var is only set in the current cmd box
you have opened with *this* syscall. this is a feature of cmd.exe... after
cmd.exe is closed (and this is - after this syscall ends) all setting to the
environment made are lost! if you'd like to change a global machine var on the
box you are forced to use other ways... there are tools like "setx" (microsoft
resource kit) and some other registry ways for setting a machine environment var...

Marc

Comment 37

[Jouni Heikniemi]

Given that system() is afaig (as far as I can grep) nowadays only used in
checksetup.pl's |system("stty", "-echo")| statements which we're anyway going to
eliminate in bug 143490, I'm starting to believe this actually WORKSFORME.
Differing opinions?

Comment 38

[Jouni Heikniemi]

A closer inspection revealed that in addition to system(), also open()ing to a
pipe and using exec() require the path. My current theory is the following:

- AS Perl opens files without a shell, if they are executables pointed to by 
  full paths. 
- If the files are not executables (Perl scripts for example) running 
  them requires association db access. AS Perl spawns cmd.exe to handle the
  association stuff, but fails without the path.

This would explain why my absolute-path-dot.exe works even though it uses an
exec() in showdependencygraph.cgi.

After discussing this with justdave on IRC, the best alternative seems to be to
add a localconfig variable for setting the path to the command interpreter
(Win32 only). In checksetup.pl, we can default to %Systemroot%\System32\cmd.exe
fairly safely, as this works on the NT family (NT/2k/XP). We could then set the
path or PERL5SHELL environment variables following Activestate's recommendation
pasted into comment 30.

Taking.

Comment 39

[Cédric Caron]

you can use the ComSpec environement variable as a default for this 
localgonfig variable.

or directly using comspec without a local config variable

This give a 99.99% safe value...

Comment 40

[Jean-Sebastien Guay]

Jouni: Good to see you get a little free time to work out some (most) of the 
Win32 bugs!

I agree that cmd.exe not being in the path causes problems with opening pipes, 
that's why I put this bug as a blocker for bug 183753. :-)

I think COMSPEC is 100% accurate (if it's set to a wrong location, lots and 
lots of things are going to break when using the command line on Windows), so I 
don't think a localconfig setting is necessary, as it will probably never be 
changed. Although maybe a localconfig setting would be a good thing if the user 
ever installs a new command processor (other than cmd.exe) on the machine and 
sets it as default, so that Bugzilla will still use cmd.exe. (installing a new 
command processor is more common on Win95/98/ME than it is on NT/2k/XP, since 
the former still use DOS as a base OS)

Which reminds me, another good reason to use COMSPEC is that on Win95/98/ME 
platforms, the command processor is command.com and not cmd.exe, and 
%SystemRoot%\System32 normally doesn't contain anything (dlls are in 
%SystemRoot%\System), so the default of %SystemRoot%\System32\cmd.exe wouldn't 
work there.

Comment 41

[Jouni Heikniemi]

Yes, comspec is a good idea. Let's use that. But we'll have to store it in
localconfig, because using ENV{COMSPEC} directly doesn't work because of taint
problems (most env. variables are tainted by default). 

I'll still verify this before doing the patch, but setting ENV{COMSPEC} as the
localconfig default value sounds like the right way to do it.

Comment 42

[Bradley Baetz (:bbaetz)]

You could untaint it, although we'll need to think of the security issues in
doing so.

The real fix is to remove all those piped opens - are there that many of them left?

Comment 43

[Jouni Heikniemi]

Showdependencygraph.cgi and collectstats.pl are the only ones if I understand
correctly. But then again, at least in these administrative scripts, a new need
may arise some day. This is going to be an easy fix, so unless you have a good
plan on how to replace dot calls and static duplicates rdf generation with
non-pipe solutions (in 2.18 timeframe), this may be a good road to take. I
welcome discussion, of course :-)

Comment 44

[Jouni Heikniemi]

I played around with this a bit. I'm starting to go for the bbaetz solution
again, as it again looks like I was misled by my previous experience. Currently,
I don't know if there is a single line of code in Bugzilla that would use
system() -- it may be that bug 183753 would, but even there, we are going to
have to fix the issue with opening from a pipe first before we can even get to
debug this.

The fix for this is probably fairly trivial. I've experimented with something
like the following in globals.pl, and it seems to be a feasible approach
(although the final solution may involve localconfig as previously described):

--
if ($^O =~ /MSWin/) {
  my $winshell = $ENV{'COMSPEC'} . ' /c';
  $winshell .= ' /x' if ($winshell =~ /cmd.exe/i);
  $ENV{'PERL5SHELL'} = $winshell;
}
--

For now, finding a good solution for pipe opens comes first. If it ends up being
IPC::Run, the scope of this bug may change.

Comment 45

[Dave Miller [:justdave]]

Comment on attachment 86109 [details] [diff] [review]
patch v2

denying review based on Jouni's comment 44.  I'm assuming this is probably
bitrotted by now anyway.

Comment 46

[Jouni Heikniemi]

I'm not aware of any callsites in Bugzilla where this would be necessary; time
and patches for other bugs have made this irrelevant. I'm marking this
WORKSFORME; if there still are external executions that would need this, please
file a bug with more information and reproduction instructions (for cvs trunk
tip, please). If this still bothers some configurations globally (on every
execution), let's consider reopening this.