161634 - [ps] envvar should not be freed after PR_SetEnv

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Andrew Schultz]

this is a regression from bug 84947
http://lxr.mozilla.org/mozilla/source/gfx/src/ps/nsPostScriptObj.cpp#381

381         PR_SetEnv(envvar);
382         free(envvar);

this is bad.  Linux seems to copy by reference instead of copying the data. 
From prenv.h:

**   The caller must ensure that the string passed
**   to PR_SetEnv() is persistent. That is: The string should
**   not be on the stack, where it can be overwritten
**   on return from the function calling PR_SetEnv().
**   Similarly, the string passed to PR_SetEnv() must not be
**   overwritten by other actions of the process. ... Some
**   platforms use the string by reference rather than copying
**   it into the environment space. ... You have been warned!

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'd say this is more like critical... this is a crash or security problem
waiting to happen.

Comment 2

[tenthumbs]

PR_SetEnv is sitting on top of putenv on unix and the putenv semantics 
require that a name=value string be used as is. The address is put into 
the new environment but the string itself is *not* copied. It's used as 
is. A wonderful way to leak memory and cause crashes.

Maybe this is actually a nspr bug, nsprpub/pr/include/prenv.h says
 NSPR_API(PRStatus) PR_SetEnv(const char *string);
but, if you build mozilla with enough warnings turned on, you'll get a 
message about the function discarding 'const'. A big hint if anyone ever 
looks at warnings.

Comment 3

[Roland Mainz]

Nice catch... :)
... accepting bug.

Comment 4

[Roland Mainz]

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

Comment 5

[Roland Mainz]

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

Comment 6

[Roland Mainz]

Attachment: Patch for 2002-08-04-08-trunk

I replaced the dynamically allocated buffer with a |static| buffer - this
should be sufficient to fix the problem.

Comment 7

[Roland Mainz]

Attachment: New patch for 2002-08-04-08-trunk

The first patch does only work the first time we print, any following print
session doesn't work since we're writing to memory which is still linked to the
env var pool... - this new patch should fix that (in theory) ...

Comment 8

[Louie Zhao]

dcone, syd
can you r=

Comment 9

[dcone (gone)]

Comment on attachment 94588 [details] [diff] [review]
New patch for 2002-08-04-08-trunk

r=dcone

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 94588 [details] [diff] [review]
New patch for 2002-08-04-08-trunk

>+        PR_SetEnv("MOZ_PRINTER_NAME=dummy_value_to_make_putenv_happy");
>+
>         sprintf(envvar, "MOZ_PRINTER_NAME=%s", printername);

That needs to become an snprintf so we don't write outside our buffer, no?

Comment 11

[Louie Zhao]

Attachment: new patch

using snprintf instead sprintf for safty

Comment 12

[Louie Zhao]

Attachment: same as previous one, using diff -u

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 95354 [details] [diff] [review]
same as previous one, using diff -u

> nchars > ARG_MAX

that should be '>=', not '>' (since if your string is ARG_MAX chars it will get
truncated by one char and ARG_MAX will be the return value with recent libc
versions).  

sr=bzbarsky with that change.

Comment 14

[Boris Zbarsky [:bzbarsky]]

actaully, as Louie just noticed, that should not be "return sprintf".  And the
temporary should be PRInt32, not "int".

Comment 15

[Louie Zhao]

Attachment: final patch(r=dcone, sr=bzbarsky)

Comment 16

[Pete Zha]

checked in trunk

Comment 17

[Pete Zha]

checked in NETSCAPE_7_0_OEM_BRANCH (a=jdunn)

Comment 18

[Roland Mainz]

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

Comment 19

[Roland Mainz]

Nominating for 1.0.2-branch...

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 95361 [details] [diff] [review]
final patch(r=dcone, sr=bzbarsky)

Marking the r/sr.
a=[e-mail] for 1.0 branch.  you know the drill

Comment 21

[Roland Mainz]

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