[ps] envvar should not be freed after PR_SetEnv

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
Printing: Output
--
critical
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Andrew Schultz, Assigned: Roland Mainz)

Tracking

({fixedOEM})

Trunk
mozilla1.2alpha
x86
Linux
fixedOEM
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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!
I'd say this is more like critical... this is a crash or security problem
waiting to happen.
Severity: normal → critical

Comment 2

16 years ago
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.
(Assignee)

Comment 3

16 years ago
Nice catch... :)
... accepting bug.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha
(Assignee)

Comment 4

16 years ago
*** Bug 143480 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
Summary: envvar should not be freed after PR_SetEnv → [ps] envvar should not be freed after PR_SetEnv
(Assignee)

Comment 5

16 years ago
*** Bug 136146 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

16 years ago
Created attachment 94581 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Keywords: patch, review
(Assignee)

Comment 7

16 years ago
Created attachment 94588 [details] [diff] [review]
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) ...
Attachment #94581 - Attachment is obsolete: true

Comment 8

16 years ago
dcone, syd
can you r=

Comment 9

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

r=dcone
Attachment #94588 - Flags: review+
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?
Attachment #94588 - Flags: needs-work+

Comment 11

16 years ago
Created attachment 95345 [details] [diff] [review]
new patch

using snprintf instead sprintf for safty

Comment 12

16 years ago
Created attachment 95354 [details] [diff] [review]
same as previous one, using diff -u
Attachment #95345 - Attachment is obsolete: true
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.
Attachment #95354 - Flags: superreview+
actaully, as Louie just noticed, that should not be "return sprintf".  And the
temporary should be PRInt32, not "int".

Comment 15

16 years ago
Created attachment 95361 [details] [diff] [review]
final patch(r=dcone, sr=bzbarsky)

Comment 16

16 years ago
checked in trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Whiteboard: branchOEM

Updated

16 years ago
Whiteboard: branchOEM → branchOEM+

Comment 17

16 years ago
checked in NETSCAPE_7_0_OEM_BRANCH (a=jdunn)
Whiteboard: branchOEM+ → branchOEM+ fixedOEM

Updated

16 years ago
Keywords: review
Whiteboard: branchOEM+ fixedOEM → fixedOEM
(Assignee)

Comment 18

16 years ago
*** Bug 134914 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
Blocks: 168057
(Assignee)

Comment 19

16 years ago
Nominating for 1.0.2-branch...
Keywords: mozilla1.0.2
Comment on attachment 95361 [details] [diff] [review]
final patch(r=dcone, sr=bzbarsky)

Marking the r/sr.
a=rjesup@wgate.com for 1.0 branch.  you know the drill
Attachment #95361 - Flags: superreview+
Attachment #95361 - Flags: review+
Attachment #95361 - Flags: approval+

Updated

16 years ago
Keywords: mozilla1.0.2 → mozilla1.0.2+

Updated

16 years ago
Keywords: fixedOEM
Whiteboard: fixedOEM
(Assignee)

Comment 21

16 years ago
*** Bug 172726 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.