26527 2009-06-18 16:37:50 -0700 XBM decoder fails if it gets a packet boundary whilst parsing the width / height 2010-04-29 11:43:07 -0700 1 1 1 Unclassified WebKit Images 528+ (Nightly build) PC Linux RESOLVED INVALID P2 Normal --- 1 scarybeasts pkasting abarth pkasting oldest_to_newest 126810 0 scarybeasts 2009-06-18 16:37:50 -0700 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. 126812 1 31518 scarybeasts 2009-06-18 16:38:33 -0700 Created attachment 31518 Fix packet boundary parsing error in the XBM decoder 126815 2 scarybeasts 2009-06-18 16:39:53 -0700 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. 126895 3 abarth 2009-06-18 23:44:35 -0700 It seems possible to write a test that uses PHP flush() in the middle of transferring the bytes of the image. 126992 4 31518 levin 2009-06-19 12:01:53 -0700 Comment on attachment 31518 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. 127016 5 scarybeasts 2009-06-19 13:23:59 -0700 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. 127017 6 abarth 2009-06-19 13:29:06 -0700 Peter, see comment 5. *ducks* 127023 7 pkasting 2009-06-19 13:44:42 -0700 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. 127026 8 scarybeasts 2009-06-19 13:51:04 -0700 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. 127028 9 pkasting 2009-06-19 13:52:47 -0700 Many bugs left? This just landed in the non-Chromium ports a few days ago :) 157682 10 scarybeasts 2009-10-25 22:56:26 -0700 I know a bit more about WebKit testing now, so I'll take another crack at this in the absence of any objections :) 157683 11 abarth 2009-10-25 23:01:57 -0700 (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. :) 157703 12 pkasting 2009-10-26 00:37:00 -0700 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. 158056 13 scarybeasts 2009-10-26 18:51:30 -0700 Doesn't Firefox still decode XBM?? Still, no objections to removing it. 158345 14 pkasting 2009-10-27 13:55:33 -0700 (In reply to comment #13) > Doesn't Firefox still decode XBM?? Nope: https://bugzilla.mozilla.org/show_bug.cgi?id=504822 219125 15 pkasting 2010-04-29 11:43:07 -0700 This bug report has become invalid because the XBM decoder has been removed from the tree (hooray!). 31518 2009-06-18 16:38:33 -0700 2009-06-19 12:01:53 -0700 Fix packet boundary parsing error in the XBM decoder xbm.diff text/plain 1616 scarybeasts SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NDgzMikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTIgQEAKKzIwMDktMDYtMTggIENocmlzIEV2YW5zICA8c2NhcnliZWFzdHNAZ21h aWwuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg ICogcGxhdGZvcm0vaW1hZ2UtZGVjb2RlcnMveGJtL1hCTUltYWdlRGVjb2Rlci5jcHA6IEZpeCBp bWFnZQorICAgICAgICBjb3JydXB0aW9uIGlmIGEgcGFja2V0IGJvdW5kYXJ5IG9jY3VycyBpbiB0 aGUgbWlkZGxlIG9mIGFuIGludGVnZXIKKyAgICAgICAgdmFsdWUuIE9ubHkgY29uc3VtZSB0aGUg d2lkdGggLyBoZWlnaHQgaW50ZWdlciB2YWx1ZXMgaWYgd2UncmUgbm90CisgICAgICAgIHJpZ2h0 IHVwIGFnYWluc3QgYSBwYWNrZXQgYm91bmRhcnkuCisKIDIwMDktMDYtMTggIFBldGVyIEthc3Rp bmcgIDxwa2FzdGluZ0Bnb29nbGUuY29tPgogCiAgICAgICAgIEZpeCBidWlsZCBidXN0YWdlLgpJ bmRleDogV2ViQ29yZS9wbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy94Ym0vWEJNSW1hZ2VEZWNvZGVy LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL3hibS9Y Qk1JbWFnZURlY29kZXIuY3BwCShyZXZpc2lvbiA0NDgyMSkKKysrIFdlYkNvcmUvcGxhdGZvcm0v aW1hZ2UtZGVjb2RlcnMveGJtL1hCTUltYWdlRGVjb2Rlci5jcHAJKHdvcmtpbmcgY29weSkKQEAg LTExOCw2ICsxMTgsMTMgQEAgYm9vbCBYQk1JbWFnZURlY29kZXI6OmRlY29kZUhlYWRlcigpCiAg ICAgICAgICAgICAgICAgICAgJndpZHRoLCAmaGVpZ2h0LCAmY291bnQpICE9IDIpCiAgICAgICAg ICAgICByZXR1cm4gZmFsc2U7CiAKKyAgICAgICAgLy8gRW5zdXJlIHdlIGFyZSBub3QgYnJlYWtp bmcgYW4gaW50ZWdlciBpbnRvIHR3byBwaWVjZXMgLS0gdGhpcyBtYXkKKyAgICAgICAgLy8gb2Nj dXIgaWYgdGhlIGlucHV0IGJ1ZmZlciBlbmRzIHJpZ2h0IGF0IHRoZSBsYXN0IGJ5dGUgb2YgaW50 ZWdlcgorICAgICAgICAvLyB3ZSBwYXJzZWQuIFRoZSAtMSBpcyBiZWNhdXNlIG1feGJtU3RyaW5n LnNpemUoKSBpbmNsdWRlcyB0aGUKKyAgICAgICAgLy8gdGVybWluYXRpbmcgTlVMTCBieXRlLgor ICAgICAgICBpZiAoY291bnQgPT0gKG1feGJtU3RyaW5nLnNpemUoKSAtIG1fZGVjb2RlT2Zmc2V0 KSAtIDEpCisgICAgICAgICAgICByZXR1cm4gZmFsc2U7CisKICAgICAgICAgLy8gVGhlIHdpZHRo IGFuZCBoZWlnaHQgbmVlZCB0byBmb2xsb3cgc29tZSBydWxlcy4KICAgICAgICAgaWYgKHdpZHRo IDwgMCB8fCB3aWR0aCA+IG1heERpbWVuc2lvbiB8fCBoZWlnaHQgPCAwIHx8IGhlaWdodCA+IG1h eERpbWVuc2lvbikgewogICAgICAgICAgICAgLy8gSWYgdGhpcyBoYXBwZW5zLCBkZWNvZGluZyBz aG91bGQgbm90IGNvbnRpbnVlLgo=