Closed
Bug 1253790
Opened 8 years ago
Closed 8 years ago
graphite2: UBSan signed integer overflow in [@graphite2::vm::Machine::run]
Categories
(Core :: Graphics: Text, defect)
Core
Graphics: Text
Tracking
()
People
(Reporter: tsmith, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: csectype-intoverflow, sec-audit, testcase, Whiteboard: [gfx-noted])
Attachments
(1 file)
1.64 KB,
application/x-font-ttf
|
Details |
This was found while fuzzing graphite2 latest revision (520d76818052772d614e581dacea69499b912be6) This issue was uncovered using Undefined Behavior Sanitizer (UBSan). More information can be found here: http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html. To reproduce: Build with UBSan enabled. run: ./gr2fonttest test_case.ttf -auto -demand /home/user/code/graphite/src/inc/opcodes.h:128:5: runtime error: signed integer overflow: -2089 * 100672259 cannot be represented in type 'int' #0 0x7f5f5ca8af93 in (anonymous namespace)::mul(unsigned char const*&, int*&, int*, regbank&) /home/user/code/graphite/src/inc/opcodes.h:128:5 #1 0x7f5f5ca87bca in graphite2::vm::Machine::run(void* const*, unsigned char const*, graphite2::Slot**&) /home/user/code/graphite/src/call_machine.cpp:121:12 #2 0x7f5f5cb25423 in graphite2::vm::Machine::Code::run(graphite2::vm::Machine&, graphite2::Slot**&) const /home/user/code/graphite/src/Code.cpp:737:13 #3 0x7f5f5cc5930a in graphite2::Pass::doAction(graphite2::vm::Machine::Code const*, graphite2::Slot*&, graphite2::vm::Machine&) const /home/user/code/graphite/src/Pass.cpp:675:17 #4 0x7f5f5cc4b00f in graphite2::Pass::findNDoRule(graphite2::Slot*&, graphite2::vm::Machine&, graphite2::FiniteStateMachine&) const /home/user/code/graphite/src/Pass.cpp:544:33 #5 0x7f5f5cc453e2 in graphite2::Pass::runGraphite(graphite2::vm::Machine&, graphite2::FiniteStateMachine&, bool) const /home/user/code/graphite/src/Pass.cpp:413:13 #6 0x7f5f5ccb7026 in graphite2::Silf::runGraphite(graphite2::Segment*, unsigned char, unsigned char, int) const /home/user/code/graphite/src/Silf.cpp:423:21 #7 0x7f5f5cbac33c in graphite2::Face::runGraphite(graphite2::Segment*, graphite2::Silf const*) const /home/user/code/graphite/src/Face.cpp:187:20 #8 0x7f5f5cae29ac in graphite2::Segment::runGraphite() /home/user/code/graphite/src/inc/Segment.h:97:45 #9 0x7f5f5cadb9a3 in (anonymous namespace)::makeAndInitialize(graphite2::Font const*, graphite2::Face const*, unsigned int, graphite2::FeatureVal const*, gr_encform, void const*, unsigned long, int) /home/user/code/graphite/src/gr_segment.cpp:46:67 #10 0x7f5f5cadb575 in gr_make_seg /home/user/code/graphite/src/gr_segment.cpp:105:24 #11 0x4f91fc in Parameters::testFileFont() const /home/user/code/graphite/gr2fonttest/gr2FontTest.cpp:684:20 #12 0x4fcc89 in main /home/user/code/graphite/gr2fonttest/gr2FontTest.cpp:787:9 #13 0x7f5f5c61cec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287 #14 0x41b985 in _start (/home/user/Desktop/graphite/gr2fonttest+0x41b985)
Comment 1•8 years ago
|
||
Will not fix. This is defined behaviour. Overflowing in these opcodes is allowed.
Updated•8 years ago
|
Whiteboard: [gfx-noted]
Comment 2•8 years ago
|
||
(In reply to martin_hosken from comment #1) > Will not fix. This is defined behaviour. Overflowing in these opcodes is > allowed. Integer overflow is explicitly defined to be undefined behavior. Please reconsider.
Flags: needinfo?(martin_hosken)
Comment 3•8 years ago
|
||
We don't want to slow the engine down by inserting a check and are quite happy if the multiplication overflows/wraps the 32-bit value. If you have a suggestion on how to code that in C++ satisfactorily, I'm open to it.
Flags: needinfo?(martin_hosken)
Reporter | ||
Comment 4•8 years ago
|
||
You can use an unsigned integer instead. Overflowing is defined behavior of unsigned integers.
Comment 5•8 years ago
|
||
(In reply to Tyson Smith [:tsmith] from comment #4) > You can use an unsigned integer instead. Overflowing is defined behavior of > unsigned integers. That should be OK for the +, - and * binops: if you cast to uint32_t, perform the operation, and cast the result back to int32_t, I think (please double-check me!) you'll end up with the same thing as the "obvious" result you'd expect from simple wrapping, without entering undefined territory. That won't work for division, though, so you can't simply package that into the binop() macro and be done; you'll need to keep division working with signed values, but fortunately that won't run into overflow so it's OK.
Comment 6•8 years ago
|
||
Alternatively, if you cast one of the operands to int64_t, the operation can't overflow; then truncate the result back to 32 bits to store it.
Comment 7•8 years ago
|
||
Fixed? in 56671221b974024dd96cc9c6f592678ee6d24841
Reporter | ||
Comment 8•8 years ago
|
||
Verified with graphite revision 56671221b974024dd96cc9c6f592678ee6d24841
Reporter | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Group: gfx-core-security → core-security-release
Comment 9•8 years ago
|
||
Graphite2 has been updated to 1.3.8 on all the relevant branches including ESRs
status-firefox45:
--- → wontfix
status-firefox46:
--- → fixed
status-firefox47:
--- → fixed
status-firefox48:
--- → fixed
status-firefox-esr38:
--- → fixed
status-firefox-esr45:
--- → fixed
tracking-firefox-esr38:
--- → 46+
tracking-firefox-esr45:
--- → 46+
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•