194761 - [ps] Huge static buffer in gfx/src/ps/nsPostScriptObj.cpp

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Kenneth Herron]

The fix for bug 161634 introduced a static buffer into nsPostScriptObj.cpp,
named "envvar". This buffer is used to hold an environment variable string, i.e.
"MOZ_PRINTER_NAME=<name of printer>".

The buffer is of size ARG_MAX from <limits.h>. ARG_MAX is the maximum size of
argument & environment data passed to a new process via exec(). On solaris
ARG_MAX is 1,048,320 bytes (double that in the 64-bit environment); on
(Mandrake) linux it's 131,072 bytes. This is wasteful for a buffer expected to
contain a single environment variable.

Comment 1

[Kenneth Herron]

Attachment: Smaller buffer for MOZ_PRINTER_NAME

This patch shrinks the environment variable buffer to be big enough for a
64-character printer name. Allocating this buffer dynamically seems more
trouble than it's worth. The code already avoided overrunning the buffer or
truncating the printer name, so the only shortcoming is that really long
printer names aren't supported.  You could double or quadruple the buffer here
and still save space over the existing code.

The patch also reduces the number of literal strings required.

> size nsPostScriptObj.o.*
nsPostScriptObj.o.new: 200497 + 2920 + 88 = 203505
nsPostScriptObj.o.old: 200597 + 2920 + 1048324 = 1251841

Comment 2

[Boris Zbarsky [:bzbarsky]]

Doh.  :(  my bad.....

Comment 3

[Alec Flett]

good catch.. the fix seems ok but it seems like somehow using an nsCAutoString
somewhere would be better...(obviously this is static so we'd need some logic
around it)

Comment 4

[Kenneth Herron]

Attachment: 2nd try - embrace mozilla's string classes

This uses a static nsCAutoString to store the environment variable. It compiles
& links fine, but I'm inexperienced with mozilla's string classes so there
could be some newbie mistakes.

> size nsPostScriptObj.o
200937 + 2920 + 88 = 203945

Comment 5

[Alec Flett]

Comment on attachment 115538 [details] [diff] [review]
2nd try - embrace mozilla's string classes

this looks fine. just for everyone reading, static classes are ok when they
appear inside functions because they don't have to be initialized by the
linker.
sr=alecf

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 115538 [details] [diff] [review]
2nd try - embrace mozilla's string classes

Dunno if dcone will ever even look at this.. r=me if you need it.

Comment 7

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

static classes really aren't very good if we ever want to unload libraries,
since some old-fashioned compilers (gcc 2.x on x86/Linux and probably some other
platforms, when not using -fuse-cxa-atexit) call the destructors using |atexit|.

Comment 8

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

(Also note that a |static nsCAutoString| would likely get a different buffer for
different calls to this function.  That's probably bad as well.)

Comment 9

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

Comment on attachment 115402 [details] [diff] [review]
Smaller buffer for MOZ_PRINTER_NAME

sr=dbaron on this patch (the first), although it might be good to:
 * move |printer_env|  and |printer|pat| down closer to where they're used.

I'm also not sure whether the %s is as good as
dummy_value_to_make_putenv_happy, or why that was needed at all.

Comment 10

[Matthias Versen [:Matti]]

Kenneth Herron: Can you attach a new patch that fixes comment#9 ?
You got r= from bz with comment #6

Comment 11

[Kenneth Herron]

Attachment: 3rd try - Dynamically allocate space for the envvar

This patch dynamically allocates a buffer for the new environment string, puts
the new buffer in the environment, and then frees any previously allocated
buffer. Any size printer name is supported without wasting any memory.

This design is based on comment #7. Unloading the library would invalidate any
static buffers defined within the library; this would be bad news if one of
those buffers was hooked into the environment. But a dynamically allocated
string would remain valid.

I gather that unloading libraries isn't supported right now so I haven't
addressed the potential memory leak in that event. I'm not sure what could be
done anyway. NSPR doesn't document any method to remove an envvar from the
environment without replacing it with another envvar. So after the envvar has
been created, some kind of buffer would have to be maintained for the life of
the process.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 122355 [details] [diff] [review]
3rd try - Dynamically allocate space for the envvar

Fix the indent on that error return statement, and r=bzbarsky... but please
file a bug on NSPR about the inability to properly clean up env vars, and
reference that bug in the code comments here and this bug in the opening
comment of the NSPR bug; once that's fixed we should update this code.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 122355 [details] [diff] [review]
3rd try - Dynamically allocate space for the envvar

> #include <limits.h>

Does limits.h still need to be included?

Comment 14

[Kenneth Herron]

Attachment: Dynamically allocate space - removed <limits.h>

Christopher is correct; <limits.h> is no longer needed. This also fixes the
stray tab character.

Bug 204494 is an RFE to add NSPR support for removing environment variables.

Comment 15

[Roland Mainz]

Reassigning to Kenneth Herron...

Comment 16

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

Comment on attachment 122505 [details] [diff] [review]
Dynamically allocate space - removed <limits.h>

Initially I didn't like this approach, but I guess it's ok.  sr=dbaron.

Comment 17

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

Checking in gfx/src/ps/nsPostScriptObj.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v  <--  nsPostScriptObj.cpp
new revision: 1.90; previous revision: 1.89
done

Comment 18

[Kenneth Herron]

Verified fixed. Thanks for checking this in, Christian.