Bug 690444 - x11alpha can crash system on certain ps files.
Summary: x11alpha can crash system on certain ps files.
Status: RESOLVED FIXED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: X Display Driver (show other bugs)
Version: 0.00
Hardware: PC Linux
: P4 normal
Assignee: Default assignee
URL:
Keywords: bountiable
Depends on:
Blocks: 690571
  Show dependency tree
 
Reported: 2009-04-22 15:19 UTC by James Cloos
Modified: 2010-07-30 09:48 UTC (History)
2 users (show)

See Also:
Customer:
Word Size: ---


Attachments
bug690444.patch (6.14 KB, patch)
2009-08-23 06:28 UTC, Boris Ananine
Details | Diff
same patch, in unified format (5.71 KB, patch)
2009-12-03 11:56 UTC, Hin-Tak Leung
Details | Diff
bug690444.patch (5.62 KB, patch)
2009-12-04 00:55 UTC, Boris Ananine
Details | Diff
Proposed patch (4.95 KB, patch)
2010-07-07 12:09 UTC, Boris Ananine
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Cloos 2009-04-22 15:19:48 UTC
I’ve noticed that with 8.64 the x11alpha driver by default tries to honour
resolution settings in the ps file.  The result is an unusably large window and
excessive VM allocations which can bog down the system enough to require a reboot.

A good example is attachment #4954 [details] from bug #690435 which does this:

featurebegin{
%%BeginFeature: *StpQuality Standard
<</HWResolution[720 360]/cupsRowFeed 4>>setpagedevice
%%EndFeature
}featurecleanup
featurebegin{
%%BeginFeature: *Resolution 2880x1440dpi
<</HWResolution[2880 1440]/cupsCompression 8>>setpagedevice
%%EndFeature
}featurecleanup

Depending on which gets used, that is either 100 M or 400 M of VM allocated by
GS.  Plus whatever the X server allocates.  And a non-scrollable window which is
many times the size of the actual display.

I do not remember this occurring with older versions of GS.

Explicitly specifying the -r option to gs(1) seems to bypass the setpagedevice-
triggered changes.
Comment 1 Ray Johnston 2009-04-23 13:25:45 UTC
The command line -r option works because (as documented) it locks in the
resolution and ignores attempts to change it.
Comment 2 Ralph Giles 2009-07-30 14:32:53 UTC
I dunno, I drag giant windows around sometimes. Depending on the system anyway,
some servers/window managers seem to clip to the display size.

What would you like us to do? The x11 device is quite primitive; applications
like gv pass in a target size.
Comment 3 James Cloos 2009-08-15 18:20:53 UTC
[Apologies for the delay in replying -JimC]

> What would you like us to do?

Not blow up the box. &#9786;

I’d be happy were not specifying -r at all to be the same in every respect as
specifying it with its default value (100, I believe), just like what gs used to do.

IOW, I suppose, ignore /HWResolution setpagedevice calls when using GUI terminals.
Comment 4 Ray Johnston 2009-08-15 19:49:58 UTC
AFAIK, Ghostscript has always honored the HWResolution option for all devices.

That is what is documented.

If a spcific device has a limitation, it can examine the parameters and reject
the 'put_params' call. For instance, some devices that have a max length or
width will do this (e.g. 32767 limit for some file formats).

If there is a way to determine the maximum page size (due to either the PageSize
or HWResolution) for x11 or x11alpha devices, then the device can handle it.
Comment 5 James Cloos 2009-08-16 00:31:32 UTC
The x11 devices never used to change their window size once it had opened until
*very* recently.  I want to say 8.64 was the first version (it was the first I
saw it in), but I’m not sure whether I ever used 8.63 to read a document which
used setpagedevice.  Probably, but I cannot be certain.

Certainly 7.x kept a static window size.  And, I think, 8.5x.

The fact that it now does change its window size — including to sizes so large
as to crash the whole box — is the regression.

Print resolutions should simply not get used by display devices.

> If there is a way to determine the maximum page size (due to either the
> PageSize or HWResolution) for x11 or x11alpha devices, then the device
> can handle it.

X supports requests for the pixel and mm dimensions of each screen and the root
window.  Plus modern window managers configure the _WIN_WORKAREA and
_NET_WORKAREA properties on the root window to specify the area available to apps.

I find that the current default of 100dpi windows works well, though it wouldn’t
hurt to be able to set the default resolution via xrdb, so that it could vary
per server.
Comment 6 Ray Johnston 2009-08-16 09:54:12 UTC
Posting a bounty for a patch (which Ralph will review) for the methods
described in comment #5 (using _WIN_WORKAREA and/or _NET_WORKAREA and the
xrdb value for default resolution) -- or some better idea that anyone that wants
to work on this has.
Comment 7 Boris Ananine 2009-08-23 06:28:59 UTC
Created attachment 5329 [details]
bug690444.patch

Proposed patch to fix #690444.
Comment 8 Boris Ananine 2009-08-23 06:35:49 UTC
Sorry if context diff is a wrong format for Ghostscript patches, I'm new to this
subject and couldn't find any info about suggested patch style in Ghostscript
docs...
Comment 9 Boris Ananine 2009-08-23 07:01:43 UTC
BTW resolution setting for x11 driver via xrdb is already implemented (resources
"xResolution" and "yResoltion" as documented in Use.htm) if I not mistaking...
Comment 10 Ralph Giles 2009-08-24 09:49:12 UTC
Thanks! We generally prefer unified diff, but I'll cope.
Comment 11 Hin-Tak Leung 2009-12-03 11:56:05 UTC
Created attachment 5738 [details]
same patch, in unified format

converting the posted patch to unified diff for review purpose.

(I just patch it in to a local git-maintained repository and run git diff to
spit it out; can be achieve with and svn checkout and svn diff as well) 

There is a compiler warning at line 1009 of base/gdevxini.c for dw and dh being
used unintialized - indeed this seems to be an error in the patch: the
submitted patch removed the needed dw, dh initialization which is needed later.


Suggest the poster fix this and re-submit.
Comment 12 Hin-Tak Leung 2009-12-03 11:58:22 UTC
The patch poster didn't add himself to CC:.

See comment above and suggest the poster fix the problem and re-submit.
Comment 13 Boris Ananine 2009-12-04 00:55:38 UTC
Created attachment 5743 [details]
bug690444.patch

Sorry for that flaw.
Fixed.
Comment 14 Hin-Tak Leung 2010-05-02 03:12:35 UTC
Grabbing a Ralph's bugs.
Comment 15 James Cloos 2010-05-08 19:08:07 UTC
And I can confirm that testing on a 64bit f11 box using ghostscript-8.71-6.fc11.x86_64 also succeeds for me.

Further testing on the laptop confirms that stack size was the issue.  When I raised the stack size from the default of 8192 to 10240 (matching my default on that 64 bit server box) the file worked with non-x11 devices.

It still crashes, though, with any of the x11 devices, even with a stacksize of 65536, eight times the default.

But not with freetype, even with large values of -r, such as -r500.

SO it is probably not worth further investigation, given that fapi is the way forward.

But if you would like to see one of the core files, I can post one.
Comment 16 James Cloos 2010-05-08 19:17:18 UTC
Comment #15 was meant for a different bug; please ignore.
Comment 17 Hin-Tak Leung 2010-07-05 03:03:40 UTC
(In reply to comment #13)
> Created an attachment (id=5743) [details]
> bug690444.patch

Apologies for taking so long to review again, and thanks for the continual work on this bug. A few more trivial and not-so-trivial comments:

- The XA_WIN_WORKAREA/XA_NET_WORKAREA are only used in x_get_work_area() in base/gdevxini.c so probably should not in base/gdevx.h . In fact I can't think of a reason for needing those defines, because using the strings directly ( x_get_win_property(..., "_WIN_WORKAREA")) probably works, and both of them are only used exactly once.

- I would probably initialize "int workarea_width, workarea_height;" explicitly to say, 32767/32767 ( a reasonably large/impossible screen size) as the combination of 0/0 and x_get_work_area() failing or getting moved in a future change, for them to be accidentally 0/0 due to other reasons is a bit unpleasant.

- why is "static long *x_get_win_property()" and not "static unsigned char *x_get_win_property()"? There is no need for the explicit cast in "return (long *)prop;" if that's the case. Looking this more carefully, it is because the long is eventually cast'ed to an int in '*pwidth = area[2]; *pheight = area[3];'

- r_type, r_format, count should be initialized to something that's not 
r_type == XA_CARDINAL , r_format == 32 , count == 4, and check for bytes_remain  to be zero on return may be nice.

- a few brackets around "dev->width = dev->width <= area_width ?..." would be nice.

- it appears that xsize/ysize are only calculated to be used in "xsize * xdev->x_pixels_per_inch", which could be simplified as just "dev->width".
Comment 18 Hin-Tak Leung 2010-07-05 03:08:42 UTC
(In reply to comment #17)
> - why is "static long *x_get_win_property()" and not "static unsigned char
> *x_get_win_property()"? There is no need for the explicit cast in "return (long
> *)prop;" if that's the case. Looking this more carefully, it is because the
> long is eventually cast'ed to an int in '*pwidth = area[2]; *pheight =
> area[3];'

Please ignore this part of the comment - I forgot to delete it as I wrote along, when I got to writing the next part about checking r_type/etc and realise the "long" is correct as long as all the r_type/etc are checked.
Comment 19 Boris Ananine 2010-07-07 12:09:57 UTC
Created attachment 6440 [details]
Proposed patch

> - The XA_WIN_WORKAREA/XA_NET_WORKAREA are only used in x_get_work_area() in
> base/gdevxini.c so probably should not in base/gdevx.h . In fact I can't think
> of a reason for needing those defines, because using the strings directly (
> x_get_win_property(..., "_WIN_WORKAREA")) probably works, and both of them are
> only used exactly once.

Fixed.

> - I would probably initialize "int workarea_width, workarea_height;" explicitly
> to say, 32767/32767 ( a reasonably large/impossible screen size) as the
> combination of 0/0 and x_get_work_area() failing or getting moved in a future
> change, for them to be accidentally 0/0 due to other reasons is a bit
> unpleasant.

Current implementation of x_get_work_area can't fail leaving its output vars
uninitialized: they'll be assigned WidthOfScreen and HeightOfScreen of
xdev->scr respectively.
Future changes may of course rearrange the things, so if you insists on inline
initialization in calling functions, here you are. :)

> - r_type, r_format, count should be initialized to something that's not 
> r_type == XA_CARDINAL , r_format == 32 , count == 4, and check for bytes_remain
>  to be zero on return may be nice.

Fixed.

> - a few brackets around "dev->width = dev->width <= area_width ?..." would be
> nice.

Entire expression has been simplified.

> - it appears that xsize/ysize are only calculated to be used in "xsize *
> xdev->x_pixels_per_inch", which could be simplified as just "dev->width".

Fixed (in gdev_x_put_params).
Comment 20 Hin-Tak Leung 2010-07-27 22:24:30 UTC
I have tested the patch and it does what it claims to do with attachment #4954 [details] from bug #690435 . This is a useful change to have and I hope this makes the next release.

I'll pass it along for somebody else to review or commit.
Comment 21 Till Kamppeter 2010-07-30 09:48:55 UTC
Boris, thank you very much for fixing this annoying bug. For many users it is irrelevant, but for me as printing workflow developer it is very useful for debugging.

I have tested it and it works for me and Hin-Tak has reviewed it as a Ghostscript expert. So I committed it now as rev. 11556.