Bug 692569

Summary: Regression: seg fault reading PDF file
Product: Ghostscript Reporter: Marcos H. Woehrmann <marcos.woehrmann>
Component: GeneralAssignee: Robin Watts <robin.watts>
Status: NOTIFIED FIXED    
Severity: normal CC: michael.vrhel
Priority: P2    
Version: master   
Hardware: PC   
OS: Linux   
Customer: 384 Word Size: ---

Description Marcos H. Woehrmann 2011-10-05 13:41:30 UTC 
The customer reports and I've verified that Ghostsscript 9.04 and master seg fault when reading the attached PDF file.  Ghostscript 9.02 reads the file without error.

The command line I'm using:

  ./bin/gs -sDEVICE=tiffg4 -r300 -o test.tif ./DOC139new.pdf

Git bisect says:

1553ea878b414b4ac389f7cec4c2076bc52be966 is the first bad commit
commit 1553ea878b414b4ac389f7cec4c2076bc52be966
Author: Robin Watts <Robin.Watts@artifex.com>
Date:   Thu Apr 28 20:23:29 2011 +0100

    Stop compiler turning for loop into memset in halftoning code.
Comment 2 Robin Watts 2011-10-10 15:54:42 UTC 
This is actually an error in the thresholding code, rather than my change.

The thresholding code is setting one of the 'width' values to be -1 - an illegal value. Memset silently ignores this, but my code goes into an infinite loop.

Examining the cause now - may need to talk to Michael about it.
Comment 3 Robin Watts 2011-10-10 18:51:31 UTC 
The problem is that image_render_mono_ht is called with xci = -1. This causes offset_bits to be calculated as 17 (when we really expect a number 0 < offset_bits <= 15).

I can fix the problem with:

diff --git a/gs/base/gximono.c b/gs/base/gximono.c
index c3c4c07..916bb53 100644
--- a/gs/base/gximono.c
+++ b/gs/base/gximono.c
@@ -885,10 +885,13 @@ image_render_mono_ht(gx_image_enum * penum_orig, const byt                 if (penum->ht_landscape.index < 0) {
                     penum->ht_landscape.xstart = penum->xci + vdi - 1;
                     offset_bits = (penum->ht_landscape.xstart % 16) + 1;
+                    /* xci can be negative, so allow for that */
+                    if (offset_bits <= 0) offset_bits += 16;
                 } else {
                     penum->ht_landscape.xstart = penum->xci;
+                    /* xci can be negative, see Bug 692569. */
                     offset_bits = 16 - penum->xci % 16;
-                    if (offset_bits == 16) offset_bits = 0;
+                    if (offset_bits >= 16) offset_bits -= 16;
                 }
                 if (offset_bits == 0 || offset_bits == 16) {
                     penum->ht_landscape.offset_set = false;

But that leaves the question, why are we getting xci < 0 ?

Michael?
Comment 4 Robin Watts 2011-10-10 18:52:49 UTC 
The same fix will presumably be required in gxicolor.c too.
Comment 5 Michael Vrhel 2011-10-11 00:39:33 UTC 
Robin thanks for digging into this.  I will look at this first thing tomorrow
Comment 6 Robin Watts 2011-10-11 16:41:37 UTC 
Fixed in:

commit fecf36b333ddac3e469512be536a498382741003
Author: Robin Watts <Robin.Watts@artifex.com>
Date:   Tue Oct 11 17:36:48 2011 +0100

    Fix Bug 692569; SEGV in threshold halftoning code.

    penum->xci can be negative in clist rendering of landscape cases.
    The % operator in C is broken (or at least counter-intuitive) so
    the code goes wrong; fix it up with some tweaks to the logic
    afterwards.

    In a future commit, we'll move from % 16 to & 15 (does the right thing,
    and is a lot faster).

    No differences expected.