248988 - On Windows, attachments may fail due to "CGI open of tmpfile"

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[:glob ✱]

On Windows, attaches may fail due to "CGI open of tmpfile".

this is because perl's CGI.pm has the following list of paths hardcoded to use
as a temp directory:

  \usr\tmp
  \var\tmp
  C:\temp
  \tmp
  \temp
  \Temporary Items
  \WWW_ROOT
  \SYS$SCRATCH
  C:\system\temp
  $ENV{TMPDIR}
  .

note a lack of $ENV{TEMP} or $ENV{TMP}

in CGI.pl we should copy $ENV{TMP} to $ENV{TMPDIR} before loading CGI.pm

Comment 1

[:glob ✱]

when running under apache, $ENV{TEMP} and $ENV{TMP} aren't available to the cgi.

can't use Win32::API to use GetTempPath as Win32::API isn't included in
activestate perl by default.


oh, and $ENV{TMPDIR} is actually the first path checked, not the last.

Comment 2

[:glob ✱]

Attachment: set TMPDIR

Comment 3

[:glob ✱]

a fix for 2.16 is to create the C:\TEMP directory with appropiate permissions
for your web server to write to.

Comment 4

[Dave Miller [:justdave]]

Any of you folks with Windows want to give this a look-through?

This isn't blocking 2.18, but it meets the criteria for check-in during the
freeze, so if it's ready before then it'll go in.

Comment 5

[Jouni Heikniemi]

I encountered this bug months and months ago, but haven't seen it since. How
does "attaches may fail" occur in practice? What sort of error we get? How
should this patch be tested?

Also, last time I tinkered with TMPDIR (I think it was with bug 225359 and the
fork emulation patch attempt; File::Temp needed it), I encountered a set of
taint problems. Are you certain that pushing inherently tainted ENV{'TEMP'} to
TMPDIR won't cause an issue with cascading taints?

Comment 6

[:glob ✱]

(In reply to comment #5)
> How does "attaches may fail" occur in practice? 

it occurs if you don't have any of the paths listed in comment #0 writable.

on a default apache install, which runs as SYSTEM, this won't be a problem, as
CGI will write its temp directories to the bugzilla directory.

on iis however, bugzilla will be running as the IUSR account which doesn't have
write access to directories by default.

so if you're running on IIS, and haven't created c:\temp, you'll encounter this bug.

this was seen in the wild ..

http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&selm=606b4a08.0406202139.2a18be06%40posting.google.com

> What sort of error we get? 

CGI open of tmpfile: No such file or directory
or
CGI open of tmpfile: Permission denied

> How should this patch be tested?

remove permission for apache to write to the directories in the list.  note that
the current directory is in the list.

> Are you certain that pushing inherently tainted ENV{'TEMP'} to
> TMPDIR won't cause an issue with cascading taints?

i haven't encountered any taint issues when running this patch.

Comment 7

[Jouni Heikniemi]

Comment on attachment 151925 [details] [diff] [review]
set TMPDIR

I cannot test this, since I just nuked my Win32 test installation yesterday
night - but if it works for you, I'm ready to do this just on code level.

>+BEGIN {
>+    if ($^O =~ /MSWin32/i) {
>+        # Help CGI find the correct temp directory as the default list isn't Windows friendly (Bug 248988)
>+        $ENV{TMPDIR} = $ENV{TEMP} || $ENV{TMP} || "$ENV{WINDIR}\\TEMP";
>+    }

My suggestions:

1) Wrap the comment so that it doesn't exceed 80 chars.

2) Remove the last element '|| "$ENV{WINDIR}\\TEMP', since that temporary
directory doesn't exist in all forms of Windows (this W2k, for instance,
doesn't have one). It would also be confusing and a bad move to write under
sysdirs, since they can also be protected by higher powers (XP and so on). All
systems should have their TEMP or TMP variable set; why not rely on that?

3) You should use apostrophes around the environment variable names
($ENV{'TMPDIR'}) and so on.

Comment 8

[:glob ✱]

(In reply to comment #7)
> 2) Remove the last element '|| "$ENV{WINDIR}\\TEMP', since that temporary
> directory doesn't exist in all forms of Windows (this W2k, for instance,
> doesn't have one). It would also be confusing and a bad move to write under
> sysdirs, since they can also be protected by higher powers (XP and so on). All
> systems should have their TEMP or TMP variable set; why not rely on that?

because apache doesn't expose TEMP and TMP to the cgi, so i need a reasonable
fallback.  if it doesn't exist, it won't be used.

Comment 9

[:glob ✱]

Attachment: set TMPDIR v2

Comment 10

[Jouni Heikniemi]

Comment on attachment 152393 [details] [diff] [review]
set TMPDIR v2

We discussed the %windir%\temp issue on IRC. I agree with glob, it's a
reasonable fallback and yes, even my Windows has it - it was just hidden
(whoever changed my Explorer not to show hidden files... odd). 

r=jouni by inspection and on the condition that it really works ;-) I can't see
why it would break anything... The patch itself doesn't work for me ("malformed
patch on line 17"), but the changes made manually seem to be ok.

Comment 11

[:glob ✱]

can someone please check this in for me?

Comment 12

[Vlad Dascalu]

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.211; previous revision: 1.210
done

Comment 13

[:glob ✱]

for reference, i lodged this as a bug in CGI.pm

http://guest:[e-mail]/rt3/Ticket/Display.html?id=30524