168614 - [ps] TMP, TMPDIR, TEMP not honored

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P3)

Description

[John Cuzzola]

Hello,
  It would appear none of the environment variables TMP , TMPDIR, TEMP are
honored by Mozilla version 1.0.0.2002052918. It always uses /tmp. I dumped the
environment by editing the run-mozilla.sh script just before it executed
mozilla-bin:

        ## Run the program
        ##

set>/tmp/environ

        $prog ${1+"$@"}


After launching the /tmp/environ file contained: (relevent variables only:)

TEMP=/home/john/.tmpdir
TMP=/home/john/.tmpdir
TMPDIR=/home/john/.tmpdir

The /home/john/.tmpdir exists and permissions are fine.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Which exact files got placed in /tmp?  Most parts of mozilla use the directory
service for this (and that in fact looks at those environment variables), but
someone could be cheating somewhere..

Comment 2

[John Cuzzola]

I'm sorry I should have been more specific. The problem occurs when trying to
print a page. I'm using diskless clients and a ram drive for /tmp that's when I
noticed the problem. The ram drive would fill to 100% and the document would
never get printed (I would see the printing progress bar but then it would
simply disappear - Mozilla doesn't crash). As for what files are created,
Mozilla immediately deletes those temporary files so I can't be more specific.
Other than a:

watch "df -h"

(print a graphic intensive/large webpage)

You can see the /tmp usage increase until it reaches 100%(small ram drive) ...
Print Job fails .. /tmp drops back to normal.

Comment 3

[Boris Zbarsky [:bzbarsky]]

To printing, and this is critical. Sticking things in /tmp and ignoring the env
vars is a major security issue, if nothing else.

Comment 4

[Roland Mainz]

Boris Zbarskywrote:
> To printing, and this is critical. Sticking things in /tmp and ignoring the 
> env vars is a major security issue, if nothing else.

I doubt we can do anything here since this looks like a Linux/libc bug.
The PostScript module uses |tmpfile()| to obtain a file handle to a temporary
file. There is no control from within Mozilla to set the filename - this is all
"done" by libc magic.
And using another API than |tmpfile()| may result in other regressions... ;-(

Comment 5

[Roland Mainz]

Reporter:
You can use the Xprint module (see
http://mozilla.org/releases/mozilla1.0/#printing) instead of the PostScript
module. Just start one Xprt (X print server) instance on the server of the
diskless clients and direct the mozilla instances via the XPSERVERLIST env var
to that server (more details on demand...). The Xprint client side (e.g.
"mozilla" in this case) is guranteed to not create any temporary files.

Comment 6

[Roland Mainz]

Does anyone know where I can file an LXR/CVS/etc. for the current glibc sources
?
Filing a fix for thah issue against the glibc sources should be easy... :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

How about we do what every sane Mozilla component does and use the directory
service instead of using tmpfile(), eh?  This is why the directory service
exists; to encapsulate all the logic of getting the temp dir, even in the face
of platform issues like being on a Mac or having a buggy libc.  Also, known bugs
in libc do not excuse programs that fail to deal with those bugs (and this bug
is now known).

Comment 8

[Roland Mainz]

Boris Zbarsky wrote:
> How about we do what every sane Mozilla component does and use the directory
> service instead of using tmpfile(), eh?  This is why the directory service
> exists; to encapsulate all the logic of getting the temp dir, even in the face
> of platform issues like being on a Mac or having a buggy libc.

IMHO the issue should always be killed at the root of the problem, not by adding
more and more layers to hide the issue. Thanks to that Mozilla has become very
bloated and slow... ;-(

Using the directory service would require at least:
- Implement a functionality similar to |tmpfile()| in the directory service for
all platform (this does AFAIK not exist)
- Turn all calls in nsPostScriptObj.cpp&co. from |fprintf()| to whatever is
required to make the code working with the crossplatform |fprintf()|-replacement

... and I am not even sure if the directory service does handle this stuff
correctly either (for example that the directory service automagically "removes"
the tmp files if the process exists (or crashes) like |tmpfile()| does).

> Also, known bugs in libc do not excuse programs that fail to deal with 
> those bugs (and this bug is now known).

Do you volunteer to fix all Unix/Linux applications which use |tmpfile()| ? =:-)
IMHO this should be fixed at the OS layer.
Linux is AFAIK the _only_ OS which has problems here, all other non-glibc-based
POSIX-conformant OSes work fine.
For Solaris we usually choose the way of filing a bug against the OS and fix it
there instead if adding workarounds in Mozilla - why can't we do the same for
Linux ? AFAIK the situation is easier there - we don't have to wait for Sun
engineering to release a patch for the OS - we can simply create a patch for
glibc and recompile it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> that the directory service automagically "removes" the tmp files

It does not.  You have to do it yourself.  That's a very good argument not to
move to the directory service, actually.  ;)

I agree that the glibc bug should be fixed.  If it's feasible to work around it
being broken, we should do so.  Otherwise, we should close this bug out as
WONTFIX or use it to track the glibc-being-fixed state.

Comment 10

[Roland Mainz]

Boris Zbarsky wrote:
> > that the directory service automagically "removes" the tmp files
>
> It does not.  You have to do it yourself.  That's a very good argument not to
> move to the directory service, actually.  ;)

Grrrr... what about filing a bug against the directoty service ? =:-)

> I agree that the glibc bug should be fixed. 

Yeah, that would be much better. If this issue is really a security-killer we
should fix it ASAP for all glibc-based OSes and not only in our tiny mozilla
code... =:-)

> If it's feasible to work around it being broken, we should do so.

I don't like the idea of fixing it here. Than we can't say "mozilla suffers from
it, please fix it" ... =:-)

> Otherwise, we should close this bug out as
> WONTFIX or use it to track the glibc-being-fixed state.

I am not going to close this bug until someone gives me a pointer to the glibc
code that I can file a fix for that issue.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, actually... now that I think about it... The _directory_service_ does not
have a way to do this well (it can't survive a crash to clean up; it could only
clean up on restart).  But opening a stream on a file _does_ let you do this...
it's just a matter of unlink() while holding a file descriptor to the file; the
file will die as soon as you lose the descriptor.

Comment 12

[rods (gone)]

futured for now

Comment 13

[Andrew Schultz]

http://www.gnu.org/software/libc/#Bugs

Comment 14

[Roland Mainz]

Andrew Schultz wrote:
> http://www.gnu.org/software/libc/#Bugs

Where's the source code CVS ?

Comment 15

[Kenneth Herron]

Attachment: tmpfile() replacement that respects TMPDIR/TEMP/TMP

This patch adds a replacement for tmpfile(). The new function gets the
preferred temporary directory from directory services (which makes use of
TMPDIR etc.), uses tempnam() to create a temporary file in that directory. I've
tested this on solaris 8 and mandrake linux. tempnam() is a classic POSIX
function so it should be just as portable as tmpfile().

I have to disagree with comment 4. POSIX doesn't say anything about tmpfile()
(or tmpnam()) using TMPDIR, and tmpnam()/tmpfile() on solaris doesn't respect
TMPDIR. Glibc's behavior may be unfortunate but it's conformant. tempnam() on
glibc and solaris both respect TMPDIR--although this behavior isn't mandated by
POSIX either--so the glibc & solaris people are apparently aware of the issues.


So I doubt you'd get far getting either group to treat the current behavior as
a bug. Even if you could, there are other platforms and the installed base to
consider.

Comment 16

[Roland Mainz]

Comment on attachment 127276 [details] [diff] [review]
tmpfile() replacement that respects TMPDIR/TEMP/TMP

Some comments:
1. Please do _not_ use |tempnam()|, on some platforms it has nasty sideeffects
(suggested replacement is |mkstemp()|)
2. Are you really sure that |+	rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR,
getter_AddRefs(tmpdir_xpcom));| does not trigger problems on Unix-like
platforms like BeOS or MacOSX (note that we have a X11 port of Mozilla for
MacOSX)

Personally I don't like it to add code to work around minor OS issues which are
fixed in newer OS versions unless it is absolutely neccesary...

Comment 17

[Kenneth Herron]

Attachment: Use |mkstemp()| to open temporary files.

This version constructs the filename template using xpcom, then calls
|mkstemp()| to open the file.

As far as I know, the call to |NS_GetSpecialDirectory()| should return a
reasonable directory on all platforms. NS_OS_TEMP_DIR is not a unix-specific
location.

Again, I've tested this on solaris 8 and mandrake linux 9.0.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, all three functions seem to be in use in Mozilla:

http://lxr.mozilla.org/seamonkey/search?string=tempnam%5C%28
http://lxr.mozilla.org/seamonkey/search?string=tmpfile%5C%28
http://lxr.mozilla.org/seamonkey/search?string=mkstemp%5C%28

Comment 19

[Brendan Eich [:brendan]]

dbaron: looks like only Windows and OpenVMS badness using tmpnam, perusing
http://lxr.mozilla.org/seamonkey/search?string=tempnam%5C%28.  The tmpfile usage
in nsObjectFrame.cpp is a bit more worrisome.  mkstemp is the race-free way to
go, last I heard (I'm an old Unix hacker, but not that old ;-).  I'll review the
patch in a minute.

/be

Comment 20

[Brendan Eich [:brendan]]

No point in leaving this assigned to rods (gone).

/be

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 127401 [details] [diff] [review]
Use |mkstemp()| to open temporary files.


>+  char *templ = ToNewCString(templ_cstr);
>+  NS_ENSURE_TRUE(templ, NULL);
>+
>+  // Open a file based on the template.
>+  int fd = mkstemp(templ);

No need for templ -- instead of the above, do this:

+  int fd = mkstemp(PromiseFlatCString(templ_cstr).get());

If there's a problem, let us know.  My string fu is not at dbaron's level, but
I think this is right, and it can avoid a copy to a heap temporary.

>+
>+  // Convert the file descriptor into a file handle and unlink
>+  // the filesystem entry.
>+  FILE *thandle;
>+  if (-1 == fd) {

Testing (fd < 0) is faster and easier on the eyes.

Looks good otherwise, fix the above and sr=[e-mail].

/be

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The issue with the string is that mkstemp takes a char*, not const char*, and
modifies it.  You could just use NS_CONST_CAST(templ_cstr.get()) (no need for
PromiseFlatCString) if you're happy with the idea of the function messing with
the internals of an nsCAutoString -- it should all be ok, since the length isn't
being changed and nothing else has used the string.  But if that makes you
queasy, what's currently there is probably the way to go.

Comment 23

[Brendan Eich [:brendan]]

Thanks, dbaron.  I'm ok with NS_CONST_CAST here.  It's a safe application of
const_cast (for a change!).

/be

Comment 24

[timeless]

a 'quick' check makes me worry about:
Tru64, Sco5, Win32 (and i'm not sure about BeOS and the headers aren't available
from w2k)

fwiw PS doesn't build on Win32 today. I've just patched it to almost build, but
it complains about nsFontCache :(. I might get some flavor of SCO in the mail
shortly, but I don't have Tru64 coverage and i'm not sure if we care if people
on tru64 can't print 33 (magic number) documents.

Comment 25

[Kenneth Herron]

Attachment: Revised mkstemp-based tmpfile() replacement

Reworked per comments 21-23.

However, timeless has pointed out that the Tru64 platform circa 2001 has a
buggy mkstemp(); see for example
<http://www.ornl.gov/its/archives/mailing-lists/tru64-unix-managers/2001/08/msg00307.html>.
I don't have access to a Tru64 box so I can't see if this currently a problem.

If this is a concern, it wouldn't be difficult to replace mkstemp() with code
to generate the filename and open the file. This could live in the PS module,
or it could be added to eg NSPR. Currently, mkstemp() is only used in two other
places: xpinstall, which is a separate program, and the dbm module, which
supplies its own implementation.

Comment 26

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsTempfilePS.h

Based on portability concerns it seems best to avoid using any of the standard
libc functions for creating temporary files. nsPostScriptObj.cpp is over 3,000
LOC and I have future plans to separate postscript generation from print job
submission, so I've written a standalone factory class for creating files to be
used as temporaries.

This is the header for this class. It should be installed as
gfx/src/ps/nsTempfilePS.h.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

    nsresult CreateTempFile(nsILocalFile** aFile,
	    FILE **aHandle, const char *aMode);

is there a special reason to avoid PRFileDesc? hm, presumable the current ps
code uses FILE* currently...

Comment 28

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsTempfilePS.cpp

This is the implementation for the new temporary file class. It will create
files within the special directory NS_OS_TEMP_DIR, which respects TMP, TEMP,
and TMPDIR on *nix. Files are created using a unique-file creation method
supplied by nsILocalFile which prevents opening a file that already exists. I
believe it's reasonably secure, though it has two flaws:

1) |nsIFile::CreateUnique()| uses predictable filenames, so a malicious user
with write access to TMPDIR could force it to fail.

To avoid this, one normally uses a strong PRNG to generate filenames. The
mozilla code base doesn't seem to include one, and using one from libc raises
portability issues again. There's also the issue of how to seed the PRNG
reliably and efficiently from within the PS module.

2) |nsIFile::CreateUnique()| closes the file after creating it, so the caller
must reopen the file in order to access it. This creates a window of
opportunity for an attacker to replace the file with a symlink, if he has
access to delete/rename the user's files in TMPDIR.

This is normally avoided by choosing a TMPDIR that doesn't allow this, or by
opening the file once and keeping it open. nsIFile and nsILocalFile don't have
a suitable member function; nsLocalFileUnix has a |CreateAndKeepOpen()|, but it
returns a PRFileDesc* and it's not provided by any other LocalFile class.

Comment 29

[Kenneth Herron]

Attachment: Use nsTempfilePS for PS temporary files

This patch integrates nsTempfilePS into nsPostScriptObj. I've tested printing
to file, to printer, and print preview; everything works as expected and no
temp files or file descriptors are being leaked or left open. I've also tested
the VMS print-job-submission changes as well as I could; everything seems to
work there.

I tried to make as few changes as possible to avoid cluttering up this patch
with out-of-scope work. The old code used |tmpfile()| and didn't have to delete
old temp files, so it was necessary to add file-deletion calls in the
appropriate places. The print-job-submission code (both *nix and VMS) needed a
lot of changes so I chose to clean that up a bit.

Comment 30

[Kenneth Herron]

To answer comment #27, The PS module currently uses FILE* and stdio for its
postscript output. It would be simple to add a version of that function which
returns a PRFileDesc*, but right now there's no need for it.

Comment 31

[Kenneth Herron]

Comment on attachment 137809 [details]
New file gfx/src/ps/nsTempfilePS.h

Brendan, could you review some or all of these? You reviewed one of the earlier
patches. No knowledge of the postscript language is required.

Comment 32

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsTempfilePS.cpp

I've slightly altered the tempfile class implementation. With this version the
factory class creates a private subdirectory within TMPDIR, then creates all
temporary files within that directory. The private directory is removed when
the factory class is destroyed.

Comment 33

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsTempfilePS.h

The only change from the previous version is to declare a destructor which
deletes the private subdirectory.

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 137929 [details]
New file gfx/src/ps/nsTempfilePS.h

 * The Initial Developer of the Original Code is 
 * Netscape Communications Corporation.

hm, it is? and in 1998 too? in any case:
 * Version: NPL 1.1/GPL 2.0/LGPL 2.1
you should use MPL instead of NPL, and list yourself as contributor.

     * Like |nsresult CreateTempFile(nsILocalFile** aFile)|, but also

just mentioning the function name should be enough, you don't need the entire
prototype...

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 137928 [details]
New file gfx/src/ps/nsTempfilePS.cpp

(same license comments apply)

	mTempDir = nsnull;
no need for this, it's a COMPtr - they get initialized to nsnull automatically

however, you should use something like this anyway:
rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(mTempDir));

    mCount = PR_Now() % ULONG_MAX;

you can't deal with a PRTime like this. use the 64bit macros from NSPR, or use
nsInt64
(http://www.mozilla.org/projects/nspr/reference/html/prlong.html#21968)

    rv = mTempDir->AppendNative(nsPrintfCString("%lx.tmp", mCount++));
you should not assume that the native encoding is ascii compatible. yes I know
it's done in a lot of places, but the interface does not at all require that.

    nsCOMPtr<nsILocalFile> temp = new nsLocalFile();

please use do_CreateInstance, and don't include nsLocalFile.h
(contract id is @mozilla.org/file/local;1)

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

>please use do_CreateInstance, and don't include nsLocalFile.h
>(contract id is @mozilla.org/file/local;1)

actually, please use NS_NewLocalFile:
http://lxr.mozilla.org/seamonkey/source/xpcom/build/nsXPCOM.h#188

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 137814 [details] [diff] [review]
Use nsTempfilePS for PS temporary files

+  if (nsnull != mDocProlog)

other code in this file (visible in this patch) just uses  the |if
(mDocProlog)| style

indentation is slightly wrong here:
+	mPrintSetup->out = nsnull;
and here:
+	mDocProlog = nsnull;

Comment 38

[Kenneth Herron]

Attachment: Use nsTempfilePS for PS temporary files, version 2

Cleaned up tab indents and |if (nsnull != foo)| style per comments.

Comment 39

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsTempfilePS.cpp, version 3

This version contains the following changes:

Use |NS_GetSpecialDirectory()| to obtain the location of the temporary
directory. this required declaring mTempDir as an nsIFile instead of an
nsILocalFile, but it turns out that's sufficient.

Use |LL_L2UI()| to store the current time from |PR_Now()} into mCount. This
required declaring mCount as a PRUint32, which in turn meant adding casts where
mCount is used as an argument to |nsPrintfCString()|.

Use |nsIFile::Append()| instead of |nsIFile::AppendNative()| to append filename
components. Because the filename components are being created using
|nsPrintfCString()|, they must be widened into an nsAString.

Removed the VMS-specific code for the temporary directory name. It seems that
|NS_GetSpecialDirectory()| handles VMS just like Unix.

Corrected the license section. Somehow it hadn't occurred to me that there
might be explicit instructions on how to license new files :-)

Comment 40

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsTempfilePS.h, version 3

Changes:

Changed member declarations for the member variables as outlined for
nsTempfilePS.cpp.

Updated the license comment.

Tweaked the member function comments.

Forward-declare nsILocalFile instead of including nsILocalFile.h.

Comment 41

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 138182 [details]
New file gfx/src/ps/nsTempfilePS.cpp, version 3

		nsPrintfCString("%lx.tmp", (unsigned long) mCount++)));

nsPrintfCString uses nspr printf functions. compare
http://www.mozilla.org/projects/nspr/reference/html/prprf.html. 
as the l is documented to mean 32 bits, you don't need the cast.

looks good otherwise.

Comment 42

[Kenneth Herron]

Attachment: New file gfx/src/ps/nsTempfilePS.cpp, version 4

Removed the casts in the |nsPrintfCString()| calls.

Comment 43

[Kenneth Herron]

Comment on attachment 138178 [details] [diff] [review]
Use nsTempfilePS for PS temporary files, version 2

dbaron, any chance you could sr this and the two new files? The relevant
comments start with comment 26.

Comment 44

[Kenneth Herron]

Request blocking 1.7a. This patch fixes a minor security issue, and it's
blocking a dataloss bug. I've been trying to find a superreviewer for it since
12/30 without success.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsTempfilePS seems like the wrong name for the class.  If it's a factory, call
it a factory, not a file.

The advantage of mkstemp, etc., is that if the app crashes in the middle, the
file will be deleted.  This code doesn't have that property.  Would it work if
your factory function had only a function that returned a file handle and it
returned a file handle to a file that was already removed (i.e., unlinked).  Is
that going to be portable?  (Perhaps not...)

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, at least, I thought that was the point, although from reading the man page
I'm not sure.

Comment 47

[Brendan Eich [:brendan]]

open-unlink is Unix-specific (so Mac OS X supports it).  Windows loses, AFAIK,
but I welcome correction for more recent versions (which won't help Mozilla code
that targets all versions back to Win98).  We should definitely exploit
open-unlink on systems that support it, so there's no directory cleanup required
after a crash.

/be

Comment 48

[Boris Zbarsky [:bzbarsky]]

We're talking about the postscript printing module, here.  I think open/unlink
should be portable to all places that could conceivably build this module.

Comment 49

[Brendan Eich [:brendan]]

bz: right; so is OpenVMS using the postscript code?

Anyway, I'll help shepherd this patch in for 1.7a unless dbaron sees something
that rules it out (I should review too, having failed for too long to do so).

/be

Comment 50

[Boris Zbarsky [:bzbarsky]]

Hmm.. VMS should be using this, yes.  It should also support the POSIX unlink
behavior, I would think...

Comment 51

[Kenneth Herron]

FWIW, POSIX doesn't require an implementation to support unlinking an open file.
See <http://www.opengroup.org/onlinepubs/007904975/functions/unlink.html>,
noting the EBUSY error code.

I avoided using mkstemp() in the current patch because it's known to behave
badly on at least one platform; see comment 25. And I avoided unlinking files
after opening them because I'm concerned it might not work on VMS. Note that
this design choice has a big impact on the implementation; if we pre-unlink
these temp files and then discover it doesn't work on one platform, there will
be a lot of code to rewrite.

That said, there's a mkstemp-based patch attached to this bug. If that's what
you want to use, it probably just needs to be checked for bitrot.

Comment 52

[timeless]

i've built (w/ minor patches) postscript on windows. and it used to work there.

when i get my box back i'll work on fixing the printer enumerators so it works
better.

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 138178 [details] [diff] [review]
Use nsTempfilePS for PS temporary files, version 2

well, I'm really not a big fan of this solution, but sr=dbaron

Comment 54

[tor]

Checked in for kherron.

Checking in Makefile.in;
/cvsroot/mozilla/gfx/src/ps/Makefile.in,v  <--  Makefile.in
new revision: 1.49; previous revision: 1.48
done
Checking in nsPostScriptObj.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v  <--  nsPostScriptObj.cpp
new revision: 1.106; previous revision: 1.105
done
Checking in nsPostScriptObj.h;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.h,v  <--  nsPostScriptObj.h
new revision: 1.40; previous revision: 1.39
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsTempfilePS.cpp,v
done
Checking in nsTempfilePS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsTempfilePS.cpp,v  <--  nsTempfilePS.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsTempfilePS.h,v
done
Checking in nsTempfilePS.h;
/cvsroot/mozilla/gfx/src/ps/nsTempfilePS.h,v  <--  nsTempfilePS.h
initial revision: 1.1
done