Bug 52488 - Segfault in splash/Splash.cc:4501
Summary: Segfault in splash/Splash.cc:4501
Status: RESOLVED FIXED
Alias: None
Product: poppler
Classification: Unclassified
Component: splash backend (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: poppler-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-25 15:04 UTC by Octoploid
Modified: 2012-09-11 22:14 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Guard against scaleImage returning NULL (1.02 KB, patch)
2012-07-25 16:56 UTC, Octoploid
Details | Splinter Review
Updated patch (936 bytes, patch)
2012-08-02 08:04 UTC, Octoploid
Details | Splinter Review
Avoid bogus memory error for tilingPattern (1.47 KB, patch)
2012-08-12 17:30 UTC, Thomas Freitag
Details | Splinter Review
tiling pattern (223.01 KB, image/png)
2012-08-23 07:33 UTC, Thomas Freitag
Details
Avoid bogus memory error for tilingPattern (1.35 KB, patch)
2012-08-23 07:40 UTC, Thomas Freitag
Details | Splinter Review

Description Octoploid 2012-07-25 15:04:29 UTC
Splash::vertFlipImage (this=<optimized out>, img=0x0, width=88137, height=88097, nComps=<optimized out>)
    at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/splash/Splash.cc:4501
4501      for (p0 = img->data, p1 = img->data + (height - 1) * w;
(gdb) bt
#0  Splash::vertFlipImage (this=<optimized out>, img=0x0, width=88137, height=88097, nComps=<optimized out>)
    at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/splash/Splash.cc:4501
#1  0x00007ffff0bfd9c3 in Splash::drawImage (this=0x7fffe4450030, 
    src=0x7ffff0be7a60 <SplashOutputDev::tilingBitmapSrc(void*, unsigned char*, unsigned char*)>, srcData=0x7fffebffe520, srcMode=<optimized out>, 
    srcAlpha=<optimized out>, w=<optimized out>, h=88096, mat=0x7fffebffe550)
    at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/splash/Splash.cc:3494
#2  0x00007ffff0be9645 in SplashOutputDev::tilingPatternFill (this=0x7fffe4002750, state=0x7fffe4034bf0, gfx1=<optimized out>, 
    catalog=<optimized out>, str=0x7fffe4094c08, ptm=<optimized out>, paintType=1, resDict=0x7fffe445c8e0, mat=0x7fffebffe720, bbox=0x7fffe4094b98, 
    x0=-1, y0=0, x1=0, y1=1, xStep=<optimized out>, yStep=<optimized out>)
    at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/poppler/SplashOutputDev.cc:4062
#3  0x00007ffff0b724b5 in Gfx::doTilingPatternFill (this=0x7fffe44752f0, tPat=0x7fffe4094b80, stroke=<optimized out>, eoFill=<optimized out>, 
    text=<optimized out>) at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/poppler/Gfx.cc:2218
#4  0x00007ffff0b7268d in Gfx::opEOFill (this=0x7fffe44752f0, args=<optimized out>, numArgs=<optimized out>)
    at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/poppler/Gfx.cc:1851
#5  0x00007ffff0b6f546 in Gfx::go (this=this@entry=0x7fffe44752f0, topLevel=topLevel@entry=true)
    at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/poppler/Gfx.cc:716
#6  0x00007ffff0b6f988 in Gfx::display (this=0x7fffe44752f0, obj=<optimized out>, topLevel=<optimized out>)
    at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/poppler/Gfx.cc:682
#7  0x00007ffff0baead4 in Page::displaySlice (this=0x8a2420, out=0x7fffe4002750, hDPI=<optimized out>, vDPI=<optimized out>, rotate=0, 
    useMediaBox=<optimized out>, crop=<optimized out>, sliceX=-1, sliceY=-1, sliceW=-1, sliceH=-1, printing=false, abortCheckCbk=0, 
    abortCheckCbkData=0x0, annotDisplayDecideCbk=0, annotDisplayDecideCbkData=0x0)
    at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/poppler/Page.cc:519
#8  0x00007ffff0d2052b in Poppler::Page::renderToImage (this=0x7fffe4469b60, xres=97.793315267684491, yres=<optimized out>, x=-1, y=-1, w=-1, h=-1, 
    rotate=Poppler::Page::Rotate0) at /var/tmp/portage/app-text/poppler-0.20.2/work/poppler-0.20.2/qt4/src/poppler-page.cc:237
#9  0x00007ffff0d6f6b5 in PDFGenerator::image (this=0x720750, request=0x102add0)
    at /var/tmp/portage/kde-base/okular-4.8.4/work/okular-4.8.4/generators/poppler/generator_pdf.cpp:793
#10 0x00007ffff2460901 in Okular::PixmapGenerationThread::run (this=0x721cd0)
    at /var/tmp/portage/kde-base/okular-4.8.4/work/okular-4.8.4/core/generator_p.cpp:64
#11 0x00007ffff71256db in ?? () from /usr/lib64/qt4/libQtCore.so.4
#12 0x00007ffff5c18dff in start_thread (arg=0x7fffebfff700) at pthread_create.c:308
#13 0x00007ffff6311bbd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:114

This happens when I view page 38 of the attached pdf with Okular.
Comment 1 Octoploid 2012-07-25 15:08:25 UTC
Attachment is too big.
I've uploaded it here:
http://trippelsdorf.de/cud10nebsiaol.pdf
Comment 2 Octoploid 2012-07-25 16:49:42 UTC
The following patch fixes the issue for me:

diff --git a/splash/Splash.cc b/splash/Splash.cc
index b927e5e..76fdd8f 100644
--- a/splash/Splash.cc
+++ b/splash/Splash.cc
@@ -3492,8 +3492,10 @@ SplashError Splash::drawImage(SplashImageSource src, void *srcData,
       }
       scaledImg = scaleImage(src, srcData, srcMode, nComps, srcAlpha, w, h,
                             scaledWidth, scaledHeight);
-      vertFlipImage(scaledImg, scaledWidth, scaledHeight, nComps);
-      blitImage(scaledImg, srcAlpha, x0, y0, clipRes);
+      if (scaledImg) {
+        vertFlipImage(scaledImg, scaledWidth, scaledHeight, nComps);
+        blitImage(scaledImg, srcAlpha, x0, y0, clipRes);
+      }
       delete scaledImg;
     }
Comment 3 Octoploid 2012-07-25 16:56:43 UTC
Created attachment 64677 [details] [review]
Guard against scaleImage returning NULL

This patch guards all callers of scaleImage against a possible NULL return value.
Comment 4 Thomas Freitag 2012-07-28 05:36:44 UTC
(In reply to comment #1)
> Attachment is too big.
> I've uploaded it here:
> http://trippelsdorf.de/cud10nebsiaol.pdf

The link is not correct, got a 404.
Comment 5 Octoploid 2012-07-28 06:31:12 UTC
(In reply to comment #4)
> (In reply to comment #1)
> > Attachment is too big.
> > I've uploaded it here:
> > http://trippelsdorf.de/cud10nebsiaol.pdf
> 
> The link is not correct, got a 404.

Yes, sorry I've deleted that file, because it is copyrighted
and I don't want to get into trouble with Elsevier...

If you google for "cud10nebsiaol.pdf" you'll will find it.
Comment 6 Thomas Freitag 2012-07-28 09:18:04 UTC
Seems as if You get the problem on page 38, don't You?
As far as I can see Your patch the segfault, but it doesn't repair the problem itself: My implementation of tilingPatternFill runs into a problem with this page: it results in trying to create a huge bitmap with a size of more than 100.000 pixel in height and width. 
After Albert will commit my blend mode patch, I'll create a patch repairs that, too.
Comment 7 Octoploid 2012-07-28 10:55:28 UTC
(In reply to comment #6)
> Seems as if You get the problem on page 38, don't You?

Yes, see comment 0: »This happens when I view page 38 of the attached pdf with Okular.«

> As far as I can see Your patch the segfault, but it doesn't repair the problem
> itself: My implementation of tilingPatternFill runs into a problem with this
> page: it results in trying to create a huge bitmap with a size of more than
> 100.000 pixel in height and width. 
> After Albert will commit my blend mode patch, I'll create a patch repairs that,
> too.

Thanks Thomas!
Comment 8 Albert Astals Cid 2012-08-01 21:58:30 UTC
Thomas: I think i commited the patch you mention.
Comment 9 Albert Astals Cid 2012-08-01 22:04:25 UTC
Octopoid's changes make sense anyway, scaledImage can return null, so we have to protect against them.

Octoploid, before i can commit your patch i need your name so i can give you proper copyright attribution.
Comment 10 Octoploid 2012-08-02 02:46:00 UTC
(In reply to comment #9)
> Octopoid's changes make sense anyway, scaledImage can return null, so we have
> to protect against them.
> 
> Octoploid, before i can commit your patch i need your name so i can give you
> proper copyright attribution.

Markus Trippelsdorf <markus@trippelsdorf.de>

Thanks.
Comment 11 Thomas Freitag 2012-08-02 07:37:00 UTC
(In reply to comment #9)
> Octopoid's changes make sense anyway, scaledImage can return null, so we have
> to protect against them.
> 
> Octoploid, before i can commit your patch i need your name so i can give you
> proper copyright attribution.

Yes, like always You're true. And I tested a change in tilingPattern last weekend, but I wasn't complete satisfied with my changes, so probabbly it will take a while.
But it would be helpful if Markus can change his patch a little bit: 

      if (scaledImg == NULL) {
        return splashErrBadArg;
      }
in case that scaleImg is null. This would be more compatible to Splash::arbitraryTransformImage, where You, Albert, already made this change.
Comment 12 Octoploid 2012-08-02 08:04:19 UTC
Created attachment 65059 [details] [review]
Updated patch

Updated patch with Thomas' suggestion.
Comment 13 Albert Astals Cid 2012-08-02 22:49:27 UTC
Commited the protection patch, waiting for Thomas' "better" one
Comment 14 Thomas Freitag 2012-08-12 17:30:01 UTC
Created attachment 65477 [details] [review]
Avoid bogus memory error for tilingPattern

I think, I now find a solution. TilingPatterns are quite sensitive, so this solution is more a practical approach than really a theoretical solution. But I'm satisfied with the actual regtest results, and even if we get this problem under other cirumstances in futur, it now fall back to the Gfx implementation of tiling patterns.
Comment 15 Albert Astals Cid 2012-08-15 21:34:01 UTC
Thomas, I'm a bit confused, it seems that rendering the file with and without this patch have the same exact output?

Am i doing something wrong?
Comment 16 Thomas Freitag 2012-08-16 07:28:22 UTC
(In reply to comment #15)
> Thomas, I'm a bit confused, it seems that rendering the file with and without
> this patch have the same exact output?
> 
> Am i doing something wrong?

Welcome to the club :-). I was confused, too. And I tried it with returning gFalse in tilingPatternFill always, so let Gfx do the stuff: No effects either. But because I also saw no differences in what Acrobat Reader shows, I didn't care anymore. So the patch just removes the "bogus memory allocation" and doesn't change other output too much (and hopefully will bring better results in other cases)
Comment 17 Thomas Freitag 2012-08-23 07:33:58 UTC
Created attachment 66003 [details]
tiling pattern

It bothers me that we get the exact same output and therefore I digged a little bit deeper into it in my vacation: The attached png is the tiling pattern only, and due to the clipping path only a part of the white square remains and therefore it paints white on white. I guess that it was a former approach to fill the frogs, but then the graphic was moved without moving the tiling pattern, and the filling of the frogs was repaired without removing the tiling pattern itself.

But with my patch everything is now moved outside the clipping path. It doesn't matter in this case because it still changes nothing, but this could cause problems with other tiling patterns, therefore I now upload a revised patch, which still doesn't change anything in this case but is more cowardly.
Comment 18 Thomas Freitag 2012-08-23 07:40:38 UTC
Created attachment 66005 [details] [review]
Avoid bogus memory error for tilingPattern

If You have a look into this new patch, I just return gFalse if the resulting tiling pattern bitmap reaches a memory limit and so the Gfx implemention of tiling patterns will be used. I think that this is an acceptable approach also concerning performance, because it would take also a while to paint such a huge bitmap and then draw it to splash.
Comment 19 Albert Astals Cid 2012-09-11 22:14:14 UTC
Pushed, only to master as it did not seem critical.


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.