Closed Bug 1207298 Opened 5 years ago Closed 5 years ago
MSan: use-of-uninitialized-value in Read
Not sure which component this should go under so I copied bug 366559 for now. I am fuzzing the latest version of the code from: https://github.com/google/brotli
sorry, missed this -- we will look at this tomorrow
I am able to reproduce this with the latest github head. $ valgrind ./bro --decompress --input corrupt.xyzzy --output /dev/null --force shows the violators. We will get this fixed on Monday or latest on Tuesday.
We found the problem. Fix will be committed on Monday.
(In reply to Eugene Kliuchnikov from comment #4) > We found the problem. Fix will be committed on Monday. Awesome, please let me know when it lands (cset would be great) and then I'll take a snapshot to gecko to fix 1207298 1207256 and 1207667 Dan - what do you think about backports and landings as it seems plausible (but unknown) that these could impact deployed woff2
It is already landed to github (PR #183, commit 933bb9bd800c8f5f7f6a02382d33c902a98ef73a)
It is possible that the older version of woff2.0/brotli you have snapshotted has a different collection of security problems. We practically rewrote the decoder during the summer. Backports would not only fix possible security problems, but also give a significant performance boost. (about 2x speedup). If you want to backport brotli issues within woff 2.0, it is likely that you should just backport the whole woff 2.0 for simplicity. We reworked the API between woff 2.0 and brotli somewhere around April, so if your snapshot is older than that April 2015, you need to possibly get the whole woff 2.0 (not sure about this, just thinking aloud).
The new github repairs this testcase (and the ones in 1207256). Thanks! I also tested against our old snapshot (for woff2) and it did not have a problem with this testcase under valgrind.
44 (44 introduced the http encoding) will use the same code for both http encodings and woff2. Not sure whether that would impact the ease of a backport. I'm hoping that because the only known issue with the older code is 1207256 and that is sec-moderate we can just restrict the landing to nightly. Changing a whole library on a channel later in the pipeline is fraught with risk.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
If you need our help with testing the old brotli versions, we would be happy to help.
Attachment #8666811 - Flags: review?(jfkthame) → review+
It appears that WOFF2 wasn't enabled until Firefox 39 and if so we don't need to worry about it or brotli on the ESR-38 branch. Unless further testing of WOFF2 on Beta shows a pressing need to fix security bugs then your plan to let the library upgrade ride the trains is good.
https://hg.mozilla.org/integration/mozilla-inbound/rev/532703f2629fac53bd762bfd0b1704bc06ed5301 bug 1207298 - update brotli library r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/636b763d4a927f956674db242f7a7f5b822f79b6 bug 1207298 Backed out changeset 532703f2629f for build failures r=backout CLOSED TREE
one of the existing warnings in c code got promoted to an inline macro and thus popped up in directories that enforce fail on warning at compile time.
We're working on it now.
We have a fix for it. It will be pushed to github on Monday.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8881708f5c15249df6e9fefaa3fb2af5b1e98711 bug 1207298 - update brotli library r=jfkthame
(In reply to Eugene Kliuchnikov from comment #18) > We have a fix for it. It will be pushed to github on Monday. Thanks! The only real problematic part was the one warning from bit_reader.h (signed/unsigned comparison) because that gets included outside of the brotli objects (the brotli objects are whitelisted into having that warning). I was able to fix it with a one line cast and landed that knowing that the next time we do an import there will be a fix upstream already.
Whiteboard: [adv-main44-] → [adv-main44+]
You need to log in before you can comment on or make changes to this bug.