Bug 26527 – XBM decoder fails if it gets a packet boundary whilst parsing the width / height

• Status: RESOLVED
• Resolution: INVALID
• Component: Images

[Chris Evans]

Let's say we have an XBM file that starts like this:

#define dolphin_width 64
#define dolphin_height 64
static char dolphin_bits[] = {... blah

And the first 49 bytes of this file arrive in a packet, to be processed by the XBM parser. The first 49 bytes would be:

#define dolphin_width 64
#define dolphin_height 6

Looking at how the XBM decoder parses the header, we see:

        if (sscanf(&input[m_decodeOffset], "#define %*s %i #define %*s %i%n",
                   &width, &height, &count) != 2)
            return false;

In the case of the above 49 bytes, the sscanf() will be successful, leaving width==64, height==6 (should be 64) and therefore a corrupt image results.

The fix, which I will attach shortly, is to defer setting the width and the height if the sscanf() consumed the last byte of input.

[Chris Evans]

Created attachment 31518 [details]
Fix packet boundary parsing error in the XBM decoder

[Chris Evans]

Regrettably, I'm swamped at the moment. So I'm going to have to leave the patch with you. In particular, I didn't provide a test and do not anticipate having resources to do this in the short to medium term.

[Adam Barth]

It seems possible to write a test that uses PHP flush() in the middle of transferring the bytes of the image.

[David Levin]

Comment on attachment 31518 [details]
Fix packet boundary parsing error in the XBM decoder

As Adam indicated, this needs a layout test -- http://webkit.org/coding/contributing.html


Also, please add the link to the bug in the ChangeLog.

[Chris Evans]

As per previous comment, I don't have resource to drive this one further at this time. I didn't intent to mark the bug as Review:? -- sorry about that.

I was hoping that by doing the tricky work (diagnosing subtle issue and creating patch), we can just leave this bug open until the WebKit image-decoders maintainer sees it and pulls it in.

[Adam Barth]

Peter, see comment 5.  *ducks*

[Peter Kasting]

Yeah, I despise the sscanf crap this was written with.  Chris' fix is fine in the short term but really we need to just rewrite the parser here.

[Chris Evans]

Does anyone care about XBM any more? I'd say it's not worth the effort of a rewrite; might as well panel-beat the thing into shape with bugfixes and leave it? I can't imagine there are many bugs left.

[Peter Kasting]

Many bugs left?  This just landed in the non-Chromium ports a few days ago :)

[Chris Evans]

I know a bit more about WebKit testing now, so I'll take another crack at this in the absence of any objections :)

[Adam Barth]

(In reply to comment #10)
> I know a bit more about WebKit testing now, so I'll take another crack at this
> in the absence of any objections :)

No objections!  Go for it.  :)

[Peter Kasting]

The XBM decoder is slated to be ripped out entirely in bug 27823 (other browsers no longer decode this format).  Don't waste your time on it.

[Chris Evans]

Doesn't Firefox still decode XBM??
Still, no objections to removing it.

[Peter Kasting]

(In reply to comment #13)
> Doesn't Firefox still decode XBM??

Nope: https://bugzilla.mozilla.org/show_bug.cgi?id=504822

[Peter Kasting]

This bug report has become invalid because the XBM decoder has been removed from the tree (hooray!).