368668 - coverity CID: 1106 deadcode at cairo-cff-subset.c:encode_integer

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[georgi - hopefully not receiving bugspam]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/gfx/cairo/cairo/src/cairo-cff-subset.c&rev=1.1&mark=167,171#168

 [167]                  } else if (i >= -1131 && i <= -108) {

 168                      i = -i - 108;
 169 vladimir 1.1         *p++ = (i >> 8)+ 251;
 170                      *p++ = i & 0xff;

 [171]                  } else if (i >= -1131 && i <= -108) {

if the condition at [167] is false the condition at [171] is also false.
if the condition at [167] is true the [171] is not reached because of else.

in addition this function seems to do uselesss operation on |i|.



 172                      *p++ = 28;

Comment 1

[(no longer active)]

What should a patch do here?  Just remove the "else if" at line 171?

Comment 2

[georgi - hopefully not receiving bugspam]

(In reply to comment #1)
> What should a patch do here?  Just remove the "else if" at line 171?
> 

not quite sure, can't get the context. it is possible that the condition must be different and the block present.

Comment 3

[u285178]

I have fixed this bug in cairo 1.5.1
http://gitweb.freedesktop.org/?p=cairo;a=commit;h=f52aa4c13e91339e575ca2c52c9e3a1f4d95b106

Line 171 should be 
    } else if (i >= -32768 && i <= 32767) {

The effect of this bug is that some integers are encoded with five bytes instead of three resulting in a slightly larger subsetted font.

Comment 4

[(no longer active)]

Attachment: Patch to fix the code based on the cairo project fix

Trivial fix based on <http://gitweb.freedesktop.org/?p=cairo;a=blobdiff;h=55e0f1575aa9eb79a32ddd22c26722422db5c536;hp=2adc709c0bdef6e91ec9efe0234b5ba6b17122ce;hb=f52aa4c13e91339e575ca2c52c9e3a1f4d95b106;f=src/cairo-cff-subset.c>.

Comment 5

[Phil Ringnalda (:philor)]

Near as I can tell from the clear-as-mud approval rules, you need approval1.9+ before you're checkin-needed.

Comment 6

[Stuart Parmenter]

vlad: can you approve this/push it upstream?

Comment 7

[u285178]

(In reply to comment #6)
> vlad: can you approve this/push it upstream?

The patch is my patch from upstream.

As the bug doesn't actually break anything (the effect is about a 1% increase in embedded font size in PDFs) I would suggest waiting for the next cairo upgrade to pull in the fix.

Comment 8

[(no longer active)]

(In reply to comment #7)
> As the bug doesn't actually break anything (the effect is about a 1% increase
> in embedded font size in PDFs) I would suggest waiting for the next cairo
> upgrade to pull in the fix.

So, should we close this bug?

Comment 9

[Phil Ringnalda (:philor)]

Yes, fixed by bug 395449.