Bug 37189

Summary: patch to fix a bad memory access when a character starts to the left of the image
Product: poppler Reporter: William Bader <williambader>
Component: splash backendAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: williambader
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Sample file to show the problem
patch to fix the problem

Description William Bader 2011-05-13 18:58:16 UTC
Created attachment 46698 [details]
Sample file to show the problem

When Splash::fillGlyph2() draws a character, if the starting X location is negative, it increments the pointer to the glyph data to start at the position in the glyph data that should go at X position 0.
If glyph->aa is set, the glyph data is one byte per pixel, and the increment by abs(x start) is correct.
If glyph->aa is not set, the glyph data is one byte per 8 pixels, and the increment should be by abs(x start)/8 and the loop that scans the data should be offset by abs(x start)%8.
Splash::fillGlyph2() currently adds the same increment of abs(x start) whether or not glyph->aa is set, and when glyph->aa is not set, drawing a character with a negative X position can cause a bad memory reference.  For example, running
  pdftops -eps -level1sep testimagemask.pdf test.ps
under valgrind reports
==12606== Conditional jump or move depends on uninitialised value(s)
==12606==    at 0x8120237: Splash::fillGlyph2(int, int, SplashGlyphBitmap*, bool) (Splash.cc:1856)
==12606==    by 0x811FA58: Splash::fillChar(double, double, int, SplashFont*) (Splash.cc:1724)
Comment 1 William Bader 2011-05-13 19:01:26 UTC
Created attachment 46699 [details] [review]
patch to fix the problem
Comment 2 William Bader 2011-05-13 19:04:27 UTC
The negative character is the "T" in "Tiffany" at the lower left of the pdf.

The patch also changes the case for a negative y start, but I do not have a pdf to test it.
Comment 3 Albert Astals Cid 2011-05-14 05:23:13 UTC
Thanks for the patch, at the moment i don't have access to a fast enough machine to run the regression testing so it'll have to wait a bit until i test and integrate the patch.
Comment 4 Albert Astals Cid 2011-07-21 15:14:36 UTC
I'm a bit confused on the 
alpha0 = (xShift > 0? (p[xx / 8] << xShift) | (p[xx / 8 + 1] >> (8 - xShift)): p[xx / 8]);
constructs.

If as you say when glyph->aa is false we use one bit per pixel aren't we using two pixels by using the | operator?
Comment 5 William Bader 2011-07-21 15:33:05 UTC
> If as you say when glyph->aa is false we use one bit per pixel aren't we using
> two pixels by using the | operator?

I tried a few things. The code in the patch seemed to work without doing a bad memory access or making a corrupt image.
I think that it has 1 bit per pixel but 8 bits (pixels) per byte.
The patch forms a byte with xShift bits from the high side of p[xx/8] and 8-xShift bits from the low side of p[xx/8+1].  The later test for alpha0 & 0x80 extracts a single bit each time around the loop.  The loop cycles through the bits with alpha0 <<= 1 at the end of each iteration.

William
Comment 6 Albert Astals Cid 2011-07-25 14:43:52 UTC
Patch is in :-)
Comment 7 William Bader 2011-07-25 17:51:52 UTC
Thanks!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.