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.
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. ***