Summary: | sgirgb device segfaulted with attachment 3609 | ||
---|---|---|---|
Product: | Ghostscript | Reporter: | Hin-Tak Leung <htl10> |
Component: | Other Driver | Assignee: | Ray Johnston <ray.johnston> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | werner |
Priority: | P4 | Keywords: | bountiable |
Version: | master | ||
Hardware: | PC | ||
OS: | Linux | ||
Customer: | Word Size: | --- | |
Attachments: |
Patch
Revised patch minor white-space removal of the revised patch |
Description
Hin-Tak Leung
2008-09-02 16:56:22 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(). 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.
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. Apologies, the comment about endianness was rushed and wrong - still the comment on separation of bug fix and cleanups applies. 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). 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. 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. 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. 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()).
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.
Patch looks OK. I committed this as rev. 9490. Alex, please ask Wendy to submit request for the bounty. *** Bug 690560 has been marked as a duplicate of this bug. *** |