Bug 89200

Summary:	[patch] fix uninitialized variable in Splash::pipeRun(SplashPipe*) (Splash.cc:470)
Product:	poppler	Reporter:	William Bader <williambader>
Component:	splash backend	Assignee:	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:	provisional patch patch to change the test from pipe->shape to pipe->usesShape

Description William Bader 2015-02-18 06:27:08 UTC

Created attachment 113598 [details] [review]
provisional patch

Running pdftops with the PDF in https://bugs.freedesktop.org/show_bug.cgi?id=88971 produces valgrind errors.

Line 470 of Splash.cc in Splash::pipeRun() has the test

if (pipe->shape && state->blendFunc && pipe->knockout && alpha0Bitmap != NULL)

but in some paths, pipe->shape is uninitialized because Splash::pipeInit() does not initialize it and then valgrind complains:

Conditional jump or move depends on uninitialised value(s)
at 0x4BF0E5: Splash::pipeRun(SplashPipe*) (Splash.cc:470)
by 0x4CD84F: Splash::blitImage(SplashBitmap*, bool, int, int, SplashClipResult) (Splash.cc:5109)
by 0x4CE0BA: Splash::drawImage(bool (*)(void*, unsigned char*, unsigned char*), void*, SplashColorMode, bool, int, int, double*, bool, bool) (Splash.cc:3752)

I think that the best is to initialize it in pipeInit(), but I am not sure if something depends on having pipeInit() leave it alone, so the attached patch just initializes shape to 0 in a few places where it could be used uninitialized.

Possibly the test at line 470 should use pipe->usesShape instead pipe->shape or test pipe->shape after testing that alpha0Bitmap is not NULL.

Comment 1 Albert Astals Cid 2015-02-21 22:57:32 UTC

yes, pipe->shape should be pipe->usesShape, i'd prefer not initializing pipe->shape, pipeInit and friends should do the correct thing, there's already a usesShape guard, initializing it is just poors man solution of accepting can't find the "proper" solution.

Comment 2 William Bader 2015-02-22 02:33:22 UTC

Created attachment 113737 [details] [review]
patch to change the test from pipe->shape to pipe->usesShape

OK, here is a new patch that changes the test from pipe->shape to pipe->usesShape.
I ran some of my test files through pdftops under valgrind, and it fixed the "Conditional jump or move depends on uninitialised value(s)" error in Splash::pipeRun(SplashPipe*) (Splash.cc:470)

Comment 3 Albert Astals Cid 2015-02-22 14:42:57 UTC

Can you attach one of those documents?

Comment 4 William Bader 2015-02-22 14:49:30 UTC

I used the document attached to the bug report referenced in the first sentence of comment 1.  I just checked that the document is still available.
You do not need any special command line options.
Just run "pdftops source.pdf source.ps".

Comment 5 William Bader 2015-02-22 14:51:16 UTC

Here is the link https://www.dropbox.com/s/yoj6h3o4irnlgun/source.pdf?dl=0

Comment 6 Albert Astals Cid 2015-02-25 14:02:26 UTC

Pushed

Comment 7 Albert Astals Cid 2015-02-25 20:54:31 UTC

I had to go back to the pipe.shape = 0 in pipeInit since what i suggested was causing a regression in eci_altona-test-suite-v2_technical2_x4.pdf

Comment 8 William Bader 2015-02-25 21:26:52 UTC

Does other code like pipe->nonIsolatedGroup use pipe->shape without setting pipe->usesShape?

Is it possible that the change from the regression was correct because pipe->shape can be set for other purposes when pipe->usesShape is not set, and the "if" that the patch changes should really not be entered if pipe->usesShape is not set even if pipe->shape is set?

Comment 9 Albert Astals Cid 2015-02-25 22:56:42 UTC

No, the change from the regression was not correct, since the previous behaviour was the same that Adobe Reader had and the one after wasn't

The rest of the questions, no idea tbh

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.