Bug 690051 - sgirgb device segfaulted with attachment 3609 [details]
Summary: sgirgb device segfaulted with attachment 3609
Status: RESOLVED FIXED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: Other Driver (show other bugs)
Version: master
Hardware: PC Linux
: P4 normal
Assignee: Ray Johnston
QA Contact: Bug traffic
URL:
Keywords: bountiable
: 690560 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-09-02 16:56 UTC by Hin-Tak Leung
Modified: 2009-11-06 09:51 UTC (History)
1 user (show)

See Also:
Customer:
Word Size: ---


Attachments
Patch (6.40 KB, patch)
2009-01-04 12:22 UTC, wendyst2
Details | Diff
Revised patch (6.66 KB, patch)
2009-01-14 22:39 UTC, wendyst2
Details | Diff
minor white-space removal of the revised patch (7.17 KB, patch)
2009-01-19 18:54 UTC, Hin-Tak Leung
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Tak Leung 2008-09-02 16:56:22 UTC
side effect of looking into bug 689595 - attachment 3609 [details] causes ghostscript 8.62
and head (svn 9061) to segfault with this:

bin/gs -dBATCH -dPARANOIDSAFER -dNOPAUSE -sDEVICE=sgirgb \
-sOutputFile=/tmp/out attachment3609 [details]

It seems the drive crashes in sgi_print_page (), either with 
gs_heap_free_object() or _IO_fwrite() depending on whether one trims away page
2,3,4.
Comment 1 Marcos H. Woehrmann 2008-09-04 10:07:20 UTC
On my MacBook Pro the command works (this is with head, r9069) but on my Linux AMD64 box it crashes 
in gs_heap_free_object().
Comment 2 wendyst2 2009-01-04 12:22:52 UTC
Created attachment 4677 [details]
Patch

Attached please find a fix for this issue. The cause of the crash was because
the original code assumed 32-bit architecture, and allocated 4 byte for each
long integer. Also, it assumed big-endian and wrote the header directly without
proper byte order. Also, I made some minor cleanups of the code.
Comment 3 Hin-Tak Leung 2009-01-13 14:29:41 UTC
Sorry, the patch appear to be wrong - it in-discriminately swap byte-order, so
the adding swapping introduced will break big-endian platform usage. You should
take endian into explit account, such as this snipplet in base/gdevbmpc.c for
your putshort and putint:

-------------------
#if arch_is_big_endian
#  define BMP_ASSIGN_WORD(a,v) a = ((v) >> 8) + ((v) << 8)
#  define BMP_ASSIGN_DWORD(a,v)\
     a = ((v) >> 24) + (((v) >> 8) & 0xff00L) +\
         (((dword)(v) << 8) & 0xff0000L) + ((dword)(v) << 24)
#else
#  define BMP_ASSIGN_WORD(a,v) a = (v)
#  define BMP_ASSIGN_DWORD(a,v) a = (v)
#endif
--------------------

suggest reworking the patch as above; also, please separate bug fixes from
"cheanups", for the purpose of reviewing.
Comment 4 Hin-Tak Leung 2009-01-13 17:21:01 UTC
Apologies, the comment about endianness was rushed and wrong - still the comment
on separation of bug fix and cleanups applies.
Comment 5 Hin-Tak Leung 2009-01-13 17:29:23 UTC
The two lines in the original code appeared to have been removed wrongly.

-       fwrite(rowsizes,sizeof(long),3*bdev->height,pstream); /* rowstarts */
-       fwrite(rowsizes,sizeof(long),3*bdev->height,pstream); /* rowsizes */

(that's why it is important to separate cleanups from bug fixes... this appeared
to have been caused by clean-ups).
Comment 6 wendyst2 2009-01-13 18:01:53 UTC
Hi Hin-Tak,

Thanks for reviewing the patch. Indeed the comments about endian was incorrect.
I am glad that you noticed it yourself.

Unfortunately the comments about 2 lines removed wrongly is also incorrect.
fseek() automatically adds the extra bytes as the original code had intended to.
So those 2 lines are redundant. Plus they wrote uninitialized buffer to the
file, which might not be a problem but not desirable anyway (to access
unintialized memory).

Thanks again.
Comment 7 Hin-Tak Leung 2009-01-13 19:54:52 UTC
apologies - but again, clean-ups and bug fixes should be separate. There were a
couple of unnecessary(?) indentation changes (which shows up in the diff) also.
Comment 8 Ray Johnston 2009-01-14 13:38:42 UTC
The lines that are indented as seen by:

$ svn diff -x -w base/gdevsgi.c > x   
$ svn diff base/gdevsgi.c > y
$ diff x y
149c149,150
< @@ -203,18 +275,12 @@
---
> @@ -202,20 +274,14 @@
>        }
152c153
<        for(rownumber=0; rownumber<bdev->height; rownumber++)
---
> -      for(rownumber=0; rownumber<bdev->height; rownumber++)
156a158
> +        for(rownumber=0; rownumber<bdev->height; rownumber++)
160c162,163
<        for(rownumber=0; rownumber<bdev->height; rownumber++)
---
> -      for(rownumber=0; rownumber<bdev->height; rownumber++)
> +       for(rownumber=0; rownumber<bdev->height; rownumber++)
165a169
> -       gs_free(pdev->memory, (char*)cur.data, cur.line_size, 1,
167c171
<         gs_free(pdev->memory, (char*)cur.data, cur.line_size, 1,
---
> +        gs_free(pdev->memory, (char*)cur.data, cur.line_size, 1,
169a174
>         gs_free(pdev->memory,
(char*)rowsizes,4,3*bdev->height,"sgi_print_page(done)");

While I prefer writing 'filler' rather than relying on a side-effect of
'fseek', I'll approve that.

I would like 'eprintf' messages and a 'rangecheck' error when the request is
rejected due to PageCount > 0. The message text can be something like:

  sgirgb format only allows one page per file.
  Use the '%d' OutputFile option to create a single file per page.

Please submit a revised patch.
Comment 9 wendyst2 2009-01-14 22:39:58 UTC
Created attachment 4722 [details]
Revised patch

Thanks for the comments. Attached please find a revised version of the patch
that removes the indentation changes, and displays error when the source file
has multiple pages. (One note though: Now it displays range check error, and an
error stack, for multiple page source file. Not exactly user-friendly. Should
we just display the message but not the range check error... And keep the
output as the first page. That's how it is displayed on Mac Finder.)
Also fixed was a compile error on Windows (replacing bzero() with memset()).
Comment 10 Hin-Tak Leung 2009-01-19 18:54:04 UTC
Created attachment 4737 [details]
minor white-space removal of the revised patch

Minor white-space removal of Wendy's revised patch. (git shows redundant
end-of-line white spaces). Please commit.

Tested alright. The result can be verfied with xv, gimp (and maybe other
programs). Interesting to note that the code until now has always been
big-endian and therefore does-not-work-correctly on the two most well-used
platforms, windows and x86 linux.
Comment 11 Ray Johnston 2009-02-19 09:14:24 UTC
Patch looks OK. I committed this as rev. 9490.

Alex, please ask Wendy to submit request for the bounty.
Comment 12 Alex Cherepanov 2009-11-06 09:51:14 UTC
*** Bug 690560 has been marked as a duplicate of this bug. ***