225686 - No way to disable Postscript/(default|.*) printer

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[David Schweiger]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20021204
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20021204

There is no way to disable the Postscript/(default|.*) printer. Patch will follow

Reproducible: Always

Steps to Reproduce:
1. Open print dialog
Actual Results:  
Postscript/default and companions are listed in the dropdown. Whatever you do -
sing, dance, pray - the **** will not go away

Expected Results:  
Postscript/default and companions are not listed in the dropdown if a certain
pref is set

XPrint can be disabled if needed by undefining XPSERVERLIST but until now the
Postscript/#? printers always occupy the dropdown

Comment 1

[David Schweiger]

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

What exactly is the goal of this patch?  Is it to hide the existence of the PS
module from the user in a build that has it? If so, why?

Comment 3

[David Schweiger]

Reasons why I had filed this bug report:
- better customisation
- better control over user actions (the PS module allows execution of any
command, being a highlevel security risk for kiosk applications)
- PS module should be disabled in absentia of a local print queue - read
LTSP/Linux kiosk project archives
- avoid DOS attacks against printers as I had reported in
http://bugzilla.mozilla.org/show_bug.cgi?id=192542 - the issue persist.in 1.6a
enabling ordinary users to trigger complete firmware hangup, making a manual
power cycle mandatory. HP customer support suggests to use gs as filter to make
the print job valid Postscript.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> better control over user actions (the PS module allows execution of any
> command, being a highlevel security risk for kiosk applications)

Is there a bug on this?  Why would kiosk apps even build the PS module if they
don't want it, though?

> read LTSP/Linux kiosk project archives

URL?

>the issue persist.in 1.6a

It's marked dup of a fixed bug.  If it's not fixed, please reopen it (and cc
[e-mail]).  It sounds like the printer in question does not handle
some L3 or L2 feature or other that we are using, and Kenneth has been thinking
about eliminating the use of such.

Thanks for explaining the motivations for this patch.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The code looks OK. But, is there a reason why it's not acceptable to just use a
custom Mozilla bulid with Postscript disabled?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135480 [details] [diff] [review]
patch

>Index: gfx/src/gtk/nsDeviceContextSpecG.cpp
>+  nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID);
>+  PRBool psPrintersDisabled = PR_FALSE;
>+  if (pPrefs) {
>+    (void) pPrefs->GetBoolPref("print.postscript.disabled", &psPrintersDisabled);

If that call failed, psPrintersDisabled now has an undefined value.  DO NOT
ignore XPCOM errors thrown by XPCOM methods.

Yes, I know the code you were modifying does the same when it gets the
"print.printer_list" pref.  Then again, that code also leaks the pref value, so
I don't know as you want to be emulating it too much (and in fact, if you have
the time and desire to fix that leak, it would be much appreciated -- it should
probably get into an nsXPIDLCString and then assign the .get() from that to
printerList if the call succeeds).

>Index: modules/libpref/src/unix/unix.js

>+pref("print.postscript.disabled",      false);

Why litter the pref system's hashtables with stuff that you have to hardcode in
the C++ anyway?  If you feel the need to put this here for documentation
purposes, comment it out or something.

Comment 7

[David Schweiger]

What the hell is a nsXPIDLCString? This is my first code contribution for
Mozilla, pls do not expect that I know the sources as good as you, Roland or
other long term Zillanians :)

Comment 8

[David Schweiger]

Attachment: patch^2

Comment 9

[David Schweiger]

Why does bugzilla block me from assigning the bug to myself? I filed the bug, I
own it. Stupid beast. 50 points from Gryffindor :)

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Why does bugzilla block me from assigning the bug to myself?

Because you have no rights as far as bugzilla is concerned?  ;)

Comment 11

[Boris Zbarsky [:bzbarsky]]

An nsXPIDLCString is a way of automatically managing string allocation. 
CopyCharPref allocates a char* buffer and returns it in the out param.  If you
look at the code you're putting your if check around it does something like:

+    printerList = PR_GetEnv("MOZILLA_POSTSCRIPT_PRINTER_LIST");
+    if (!printerList) {
+	(void) pPrefs->CopyCharPref("print.printer_list", &printerList);
+    }  
+    printerList = strdup(printerList);

Oops.  It just leaked the string that prefs allocated.  In my opinion, the code
should be more like:

+    printerList = PR_GetEnv("MOZILLA_POSTSCRIPT_PRINTER_LIST");
+    nsXPIDLCString prefsList;
+    if (!printerList) {
+	if (NS_SUCCEEDED(pPrefs->CopyCharPref("print.printer_list",
+                                             getter_Copies(prefsList)))) {
+         printerList = prefsList.get();
+       }
+    }  
+    printerList = strdup(printerList);

This way the destructor of the nsXPIDLCString will deallocate the buffer that
prefs allocated.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135492 [details] [diff] [review]
patch^2

sr=bzbarsky, but bonus points for rolling the leak fix into this patch... ;)

Comment 13

[David Schweiger]

All hail to the bugzilla dictatorship! :) Who is in charge of updating my
accounts ACL?

Comment 14

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

Can we please avoid introducing more users of the deprecated nsIPref? Please use
nsIPrefService/nsIPrefBranch.

+  PRBool psPrintersDisabled = PR_FALSE;
I'd think naming the variable psPrintersEnabled (and inversing the pref meaning)
would be more readable.

Comment 15

[David Schweiger]

Why is Mozilla using its own strings classes instead of the ISO C++ string classes?

Comment 16

[David Schweiger]

Guys, is it possible that you take only the finger I handed you and not try to
grab the whole hand and everything attached to it? WTF is
nsIPrefService/nsIPrefBranch?

Comment 17

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

they are the preferred way to read preferences.
see
http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPrefService.idl
and http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPrefBranch.idl

Comment 18

[Boris Zbarsky [:bzbarsky]]

biesi, the code is already using nsIPref; this is not introducing another user
but just using a pointer we already have hanging around, really... David, see
http://lxr.mozilla.org/seamonkey/source/modules/libpref/public/nsIPref.idl#47
for details.  I'm ok with leaving the pref stuff as-is, because if we start
asking for cleanup of code near what David is touching he's screwed -- the
printing code is some of the worst in Mozilla (we've just found a memory leak
and a use of an interface that should no longer be used in just the dozen lines
of this patch....)

David, Mozilla is using its own string classes because support for the ISO ones
was piss-poor when most of this code was written.

My sr on the existing patch stands, assuming follow-up bugs are filed on the
other issues raised here (nsIPref, memory leak, etc).  Feel free to just assign
said bugs to me.

Comment 19

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

Comment on attachment 135492 [details] [diff] [review]
patch^2

ok... rename psPrintersDisabled to psPrintersEnabled (same for the pref), and
r=biesi

Comment 20

[David Schweiger]

Attachment: patch, limited final edition GET YOUR COPY NOW! :)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Changed the all.js line to say "true" instead of false, removed the tabs this
patch introduced (don't do that!) and

Checking in gfx/src/gtk/nsDeviceContextSpecG.cpp;
/cvsroot/mozilla/gfx/src/gtk/nsDeviceContextSpecG.cpp,v  <-- 
nsDeviceContextSpecG.cpp
new revision: 1.57; previous revision: 1.56
done
Checking in modules/libpref/src/unix/unix.js;
/cvsroot/mozilla/modules/libpref/src/unix/unix.js,v  <--  unix.js
new revision: 3.88; previous revision: 3.87
done

Comment 22

[Kenneth Herron]

David, can this bug be marked resolved?

Comment 23

[Boris Zbarsky [:bzbarsky]]

If you do, see comment 18, last paragraph...

Comment 24

[David Schweiger]

fixed in 1.6b

Thanks you all for helping.

Comment 25

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

did you file the follow-up bugs per comment 18 (last paragraph)?

Comment 26

[Roland Mainz]

Erm... the patch doesn't cover the gfx/src/xlib (e.g.
nsDeviceContextSpecXlib.cpp).
Please file a patch or reassign the bug to me...

Comment 27

[Boris Zbarsky [:bzbarsky]]

Bug 230157 filed per comment 18... Next time, please do follow the review
conditions!

Over to Roland to fix xlib issues, if any.

Comment 28

[Markus Dommer]

David, using mozilla prefs is a bad idea since we have to edit the prefs fore
each account. If a kiosk user manages to switch accounts he can use the PS
module exploit again.

Comment 29

[Roland Mainz]

Attachment: Patch for 2004-01-08-trunk, Xlib port and env var override

For the log: Patches for nsDeviceContextSpec(G|Xlib).(cpp|h) can easily be
ported to the Xlib toolkit (build with "configure
--enable-default-toolkit=xlib; gmake") via copying the sources from
gfx/src/gtk/ to gfx/src/xlib/ (like % cp gfx/src/gtk/nsDeviceContextSpecG.cpp
gfx/src/xlib/nsDeviceContextSpecXlib.cpp) and rename the variable names
s/GTK/Xlib/. No magic involved... :)

Comment 30

[Roland Mainz]

bz:
Slighly offtopic... but I guess there is already a bug which asks for cleanup in
nsDeviceContextSpec(G|Xlib).* ... but AFAIK that work was deferred until the
s/nsIPref/nsIPrefBranch/ lands (the cleanup would kill a bunch of other issues
in that code anyway and looking for the remaining issues during that change may
not be hard) ...

Comment 31

[Simon Montagu :smontagu]

Comment on attachment 138668 [details] [diff] [review]
Patch for 2004-01-08-trunk, Xlib port and env var override

r=smontagu

Comment 32

[Roland Mainz]

attachment 138668 [details] [diff] [review] checked-in...

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=mozilla%2Fgfx%2F&filetype=match&who=smontagu&whotype=regexp&sortby=Date&hours=2&date=explicit&mindate=01%2F12%2F2004+00%3A00%3A00&maxdate=01%2F12%2F2004+23%3A59%3A00&cvsroot=%2Fcvsroot

... marking bug as FIXED.