Bugzilla

Comment 2

•

8 years ago

There are a number of already-known issues in 1.3.6 that have been fixed in graphite trunk; we'll be taking a new release soon (bug 1255158). So the interesting question here is whether any of the attached testcases still crash with the latest code from upstream.

Martin, Tyson: could one or both of you test these cases and note whether any of them remain unfixed?

Flags: needinfo?(jfkthame) → needinfo?(twsmith)

Reporter

Comment 3

•

8 years ago

i dont know if you have access to a later version than the github, but as of right now:

git clone https://github.com/silnrsi/graphite.git
cd graphite
CC=gcc CXX=g++ cmake -DGRAPHITE2_ASAN:BOOL=ON .
cmake -DGRAPHITE2_ASAN:BOOL=ON .
make
for i in ../crashar/*ttf ; do ./tests/examples/simple $i ; done 2>&1 | grep SUMM
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:425 graphite2::TtfUtil::DesignUnits(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:425 graphite2::TtfUtil::DesignUnits(void const*)
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:425 graphite2::TtfUtil::DesignUnits(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:810 graphite2::TtfUtil::HorMetrics(unsigned short, void const*, unsigned long, void const*, int&, unsigned int&)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:1217 graphite2::TtfUtil::LocaLookup(unsigned short, void const*, unsigned long, void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:425 graphite2::TtfUtil::DesignUnits(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:1217 graphite2::TtfUtil::LocaLookup(unsigned short, void const*, unsigned long, void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:1217 graphite2::TtfUtil::LocaLookup(unsigned short, void const*, unsigned long, void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:810 graphite2::TtfUtil::HorMetrics(unsigned short, void const*, unsigned long, void const*, int&, unsigned int&)
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:810 graphite2::TtfUtil::HorMetrics(unsigned short, void const*, unsigned long, void const*, int&, unsigned int&)
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:1217 graphite2::TtfUtil::LocaLookup(unsigned short, void const*, unsigned long, void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:1217 graphite2::TtfUtil::LocaLookup(unsigned short, void const*, unsigned long, void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:1217 graphite2::TtfUtil::LocaLookup(unsigned short, void const*, unsigned long, void const*)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:843 graphite2::TtfUtil::FindCmapSubtable(void const*, int, int, unsigned long)
SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:360 graphite2::TtfUtil::GlyphCount(void const*)

Comment 4

•

8 years ago

Thanks, that's helpful. Martin, looks like TtfUtil needs some more attention.

I have *not* verified this by testing, but it's quite likely these issues will not directly affect Firefox because the tables involved (cmap, loca, glyf, hmtx) should be validated and either fixed or rejected by OTS before ever reaching the graphite code.

(Nevertheless, they should get fixed properly in the upstream code.)

Comment 5

•

8 years ago

Hello Knud, thanks for the bug report. Could you please attach one full ASan stack trace?

ATM I am unable to reproduce these issues using graphite revision bea11149ef002af1439f0d6ffc9c1dada6394139. Are you seeing the crashes in the shared library that you compiled or possibly the one provided by your system?

Flags: needinfo?(kokanin)

martin_hosken

Comment 6

•

8 years ago

Ditto to Tyson. Cannot reproduced either with gr2fonttest or with simple. Heap use after free does not feel like a bug that I would expect to see in the engine. Of course that doesn't mean there is nothing there. But I can't get my head around when such a thing might happen. So, looking forward to Knud's analysis on this one.

Flags: needinfo?(martin_hosken)

Reporter

Comment 7

•

8 years ago

full asan trace:
=================================================================
==32660==ERROR: AddressSanitizer: heap-use-after-free on address 0x60600000efd2 at pc 0x7fd1b982ddfd bp 0x7ffdcf141290 sp 0x7ffdcf141280
READ of size 2 at 0x60600000efd2 thread T0
    #0 0x7fd1b982ddfc in graphite2::TtfUtil::DesignUnits(void const*) /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:425
    #1 0x7fd1b97ff400 in graphite2::GlyphCache::Loader::units_per_em() const /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:324
    #2 0x7fd1b97ff400 in graphite2::GlyphCache::GlyphCache(graphite2::Face const&, unsigned int) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:125
    #3 0x7fd1b97f6a41 in graphite2::Face::readGlyphs(unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:98
    #4 0x7fd1b97cb07a in load_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:54
    #5 0x7fd1b97cb07a in gr_make_face_with_ops /dev/shm/newgraphite/graphite/src/gr_face.cpp:89
    #6 0x7fd1b97cbd20 in gr_make_file_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:242
    #7 0x400e4a in main /dev/shm/newgraphite/graphite/tests/examples/simple.c:17
    #8 0x7fd1b9415a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)
    #9 0x4010c8 in _start (/dev/shm/newgraphite/graphite/tests/examples/simple+0x4010c8)

0x60600000efd2 is located 18 bytes inside of 54-byte region [0x60600000efc0,0x60600000eff6)
freed by thread T0 here:
    #0 0x7fd1b9add6aa in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x986aa)
    #1 0x7fd1b97f63f5 in graphite2::Face::Table::releaseBuffers() /dev/shm/newgraphite/graphite/src/Face.cpp:299
    #2 0x7fd1b97f63f5 in graphite2::Face::Table::~Table() /dev/shm/newgraphite/graphite/src/inc/Face.h:208
    #3 0x7fd1b97f63f5 in graphite2::Face::Table::Table(graphite2::Face const&, graphite2::TtfUtil::Tag, unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:286
    #4 0x7fd1b97faf83 in graphite2::GlyphCache::Loader::Loader(graphite2::Face const&, bool) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:250
    #5 0x7fd1b97ff09d in graphite2::GlyphCache::GlyphCache(graphite2::Face const&, unsigned int) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:118
    #6 0x7fd1b97f6a41 in graphite2::Face::readGlyphs(unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:98
    #7 0x7fd1b97cb07a in load_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:54
    #8 0x7fd1b97cb07a in gr_make_face_with_ops /dev/shm/newgraphite/graphite/src/gr_face.cpp:89
    #9 0x7fd1b97cbd20 in gr_make_file_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:242
    #10 0x400e4a in main /dev/shm/newgraphite/graphite/tests/examples/simple.c:17
    #11 0x7fd1b9415a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)

previously allocated by thread T0 here:
    #0 0x7fd1b9add9aa in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x989aa)
    #1 0x7fd1b9832617 in graphite2::FileFace::get_table_fn(void const*, unsigned int, unsigned long*) /dev/shm/newgraphite/graphite/src/FileFace.cpp:94
    #2 0x7fd1b97f6194 in graphite2::Face::Table::Table(graphite2::Face const&, graphite2::TtfUtil::Tag, unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:281
    #3 0x7fd1b97faf83 in graphite2::GlyphCache::Loader::Loader(graphite2::Face const&, bool) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:250
    #4 0x7fd1b97ff09d in graphite2::GlyphCache::GlyphCache(graphite2::Face const&, unsigned int) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:118
    #5 0x7fd1b97f6a41 in graphite2::Face::readGlyphs(unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:98
    #6 0x7fd1b97cb07a in load_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:54
    #7 0x7fd1b97cb07a in gr_make_face_with_ops /dev/shm/newgraphite/graphite/src/gr_face.cpp:89
    #8 0x7fd1b97cbd20 in gr_make_file_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:242
    #9 0x400e4a in main /dev/shm/newgraphite/graphite/tests/examples/simple.c:17
    #10 0x7fd1b9415a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)

SUMMARY: AddressSanitizer: heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:425 graphite2::TtfUtil::DesignUnits(void const*)
Shadow bytes around the buggy address:
  0x0c0c7fff9da0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0c7fff9db0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0c7fff9dc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0c7fff9dd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0c7fff9de0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c0c7fff9df0: fa fa fa fa fa fa fa fa fd fd[fd]fd fd fd fd fa
  0x0c0c7fff9e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0c7fff9e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0c7fff9e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0c7fff9e30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0c7fff9e40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
==32660==ABORTING

1 more:
=================================================================
==1003==ERROR: AddressSanitizer: attempting double-free on 0x60200000efd0 in thread T0:
    #0 0x7f185ba086aa in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x986aa)
    #1 0x7f185b7206da in graphite2::Face::Table::releaseBuffers() /dev/shm/newgraphite/graphite/src/Face.cpp:299
    #2 0x7f185b725d38 in graphite2::Face::Table::~Table() /dev/shm/newgraphite/graphite/src/inc/Face.h:208
    #3 0x7f185b725d38 in graphite2::GlyphCache::Loader::~Loader() /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:84
    #4 0x7f185b725d38 in graphite2::GlyphCache::~GlyphCache() /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:209
    #5 0x7f185b71f03c in graphite2::Face::~Face() /dev/shm/newgraphite/graphite/src/Face.cpp:75
    #6 0x7f185b71f470 in graphite2::Face::~Face() /dev/shm/newgraphite/graphite/src/Face.cpp:82
    #7 0x7f185b6f6199 in gr_make_face_with_ops /dev/shm/newgraphite/graphite/src/gr_face.cpp:92
    #8 0x7f185b6f6d20 in gr_make_file_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:242
    #9 0x400e4a in main /dev/shm/newgraphite/graphite/tests/examples/simple.c:17
    #10 0x7f185b340a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)
    #11 0x4010c8 in _start (/dev/shm/newgraphite/graphite/tests/examples/simple+0x4010c8)

0x60200000efd0 is located 0 bytes inside of 1-byte region [0x60200000efd0,0x60200000efd1)
freed by thread T0 here:
    #0 0x7f185ba086aa in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x986aa)
    #1 0x7f185b7213f5 in graphite2::Face::Table::releaseBuffers() /dev/shm/newgraphite/graphite/src/Face.cpp:299
    #2 0x7f185b7213f5 in graphite2::Face::Table::~Table() /dev/shm/newgraphite/graphite/src/inc/Face.h:208
    #3 0x7f185b7213f5 in graphite2::Face::Table::Table(graphite2::Face const&, graphite2::TtfUtil::Tag, unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:286
    #4 0x7f185b725fb1 in graphite2::GlyphCache::Loader::Loader(graphite2::Face const&, bool) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:250
    #5 0x7f185b72a09d in graphite2::GlyphCache::GlyphCache(graphite2::Face const&, unsigned int) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:118
    #6 0x7f185b721a41 in graphite2::Face::readGlyphs(unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:98
    #7 0x7f185b6f607a in load_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:54
    #8 0x7f185b6f607a in gr_make_face_with_ops /dev/shm/newgraphite/graphite/src/gr_face.cpp:89
    #9 0x7f185b6f6d20 in gr_make_file_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:242
    #10 0x400e4a in main /dev/shm/newgraphite/graphite/tests/examples/simple.c:17
    #11 0x7f185b340a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)

previously allocated by thread T0 here:
    #0 0x7f185ba089aa in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x989aa)
    #1 0x7f185b75d617 in graphite2::FileFace::get_table_fn(void const*, unsigned int, unsigned long*) /dev/shm/newgraphite/graphite/src/FileFace.cpp:94
    #2 0x7f185b721194 in graphite2::Face::Table::Table(graphite2::Face const&, graphite2::TtfUtil::Tag, unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:281
    #3 0x7f185b725fb1 in graphite2::GlyphCache::Loader::Loader(graphite2::Face const&, bool) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:250
    #4 0x7f185b72a09d in graphite2::GlyphCache::GlyphCache(graphite2::Face const&, unsigned int) /dev/shm/newgraphite/graphite/src/GlyphCache.cpp:118
    #5 0x7f185b721a41 in graphite2::Face::readGlyphs(unsigned int) /dev/shm/newgraphite/graphite/src/Face.cpp:98
    #6 0x7f185b6f607a in load_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:54
    #7 0x7f185b6f607a in gr_make_face_with_ops /dev/shm/newgraphite/graphite/src/gr_face.cpp:89
    #8 0x7f185b6f6d20 in gr_make_file_face /dev/shm/newgraphite/graphite/src/gr_face.cpp:242
    #9 0x400e4a in main /dev/shm/newgraphite/graphite/tests/examples/simple.c:17
    #10 0x7f185b340a3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x20a3f)

SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
==1003==ABORTING

Flags: needinfo?(kokanin)

Reporter

Comment 8

•

8 years ago

 ldd ./tests/examples/simple
        linux-vdso.so.1 =>  (0x00007ffd72727000)
        libasan.so.2 => /usr/lib/x86_64-linux-gnu/libasan.so.2 (0x00007ffa647e7000)
        libgraphite2.so.3 => /dev/shm/newgraphite/graphite/src/libgraphite2.so.3 (0x00007ffa64561000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ffa64196000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007ffa63f78000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007ffa63d74000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007ffa63a6b000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007ffa63854000)
        /lib64/ld-linux-x86-64.so.2 (0x00005653f3b27000)
kokanin@ubuntu:/dev/shm/newgraphite/graphite$ !gi
git log -1
commit bea11149ef002af1439f0d6ffc9c1dada6394139
Author: Tim Eves <tim_eves@sil.org>
Date:   Fri Mar 11 16:27:33 2016 +0700

    Implement test_ref in terms of valid_upto
kokanin@ubuntu:/dev/shm/newgraphite/graphite$

martin_hosken

Comment 9

•

8 years ago

I'm wondering if this is an artefact of your fuzzing process? I notice that in the second example [heap-use-after-free /dev/shm/newgraphite/graphite/src/TtfUtil.cpp:425 graphite2::TtfUtil::DesignUnits(void const*)], the stack trace deviates in gr_make_file_face, implying that that function is making two independent calls to gr_make_face_with_ops and yet in that code there is only one place where that call can be made. Likewise in the first example, the deviation occurs in Face::readGlyphs when entering new GlyphCache, for which, again there is only one call site.

I'll be the first to admit I don't know what's going on here. Perhaps there is multi-threading going on? Graphite is not multi-threadable during font loading, but if called appropriately (without -demand) can use the gr_face across multiple threads.

Comment 10

•

8 years ago

Here's my (untested) guess as to what's leading to the last double-free shown in comment 7.

There's a table that is failing its TtfUtil::CheckTable test in Face::Table::Table(), which means you explicitly call this->~Table() from within that constructor. (Face.cpp:286). That will call releaseBuffers() to free the table's _p.

But then when the GlyphCache::Loader's (compiler-generated) destructor runs, it will call the destructors of all its Face::Table members, one of which is the one that failed CheckTable and was already destroyed. Calling ~Table a second time is bad, because the object was already destroyed (from within its own constructor).

You might this this would be harmless -- after all, deleteBuffers() clears the _p pointer in the Table, so if we call deleteBuffers() again, it shouldn't get double-free'd. But that's not true, if the compiler optimizes away the _p = 0 assignment there. And it can legitimately do that because after the object is destroyed, its value (including that _p pointer) is no longer defined. So the assignment clearing _p in the destructor can be optimized away because it's a dead store; _p is becoming undefined anyway.

(See also the discussion of gcc's -fno-lifetime-dse option at https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html.)

So I suspect a simple fix here would be to replace

    if (!TtfUtil::CheckTable(n, _p, _sz))
    {
        this->~Table();     // Make sure we release the table buffer even if the table filed it's checks
        return;
    }

with

    if (!TtfUtil::CheckTable(n, _p, _sz))
    {
        releaseBuffers(); // Release the table buffer; don't actually destroy ourself with ~Table here,
                          // that will happen whenever our owner decides.
        return;
    }

in the Face::Table constructor, to release the buffer without actually destroying the object. Then its _p and _sz fields will be properly cleared, as those assignments aren't dead, and then when the Table is explicitly destroyed by its owner, or goes out of scope, and the destructor calls releaseBuffers() again, nothing untoward should happen.

Comment 11

•

8 years ago

Knud, I'm curious to know, if you build graphite with the -fno-lifetime-dse option, does that affect any of the errors you're seeing?

Flags: needinfo?(kokanin)

Reporter

Comment 12

•

8 years ago

Jonathan, well spotted, it affects all of them. Dirtily inserted here:
 grep -r no-lifetime CMakeLists.txt
    add_definitions(-fsanitize=address -fno-omit-frame-pointer -fno-lifetime-dse -g)
-> no more crashes

Flags: needinfo?(kokanin)

Comment 13

•

8 years ago

Great, thanks for testing!

Martin, I think that's it, then. In general, it can't be a good idea to call an object's destructor from within its own constructor, because once you've done that, nobody can safely do anything more with the thing -- including simply letting it go out of scope, which leads to double-destruction.

AFAICS, the fix suggested in comment 10 should avoid this.

Flags: needinfo?(twsmith) → needinfo?(martin_hosken)

martin_hosken

Comment 14

•

8 years ago

Fixed? in 18a054493bc748de715cd579986ad782ca508c2c

Flags: needinfo?(martin_hosken)

Comment 15

•

8 years ago

Thanks; so we'll expect this to be fixed in 1.3.7 when the time comes.

Status: UNCONFIRMED → NEW

Depends on: 1255158

Ever confirmed: true

Comment 16

•

8 years ago

From the comments it sounds like this doesn't affect Firefox so I'll give it a "sec-other" rating. Please clear so we can re-triage if that's incorrect.

Group: core-security → gfx-core-security

Keywords: sec-other

Comment 17

•

8 years ago

Having identified the underlying bug here, it turns out to be in code that could be reached in Firefox, I believe, leading to a potential double-free (or possibly other use-after-free situations); but whether that can lead to a serious exploit isn't clear to me.

Updated

•

8 years ago

Depends on: 1262846

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 18

•

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

status-firefox45: wontfix → disabled

Updated

•

8 years ago

status-firefox-esr38: fixed → disabled

status-firefox-esr45: fixed → disabled

Reporter

Comment 19

•

8 years ago

bounty-worthy?

Comment 20

•

8 years ago

Please follow the instructions for the bounty program on https://www.mozilla.org/security and email us at security@mozilla.org for bounty related questions.

Dan, does the sec-other rating still apply here based on comment 17?

Flags: needinfo?(dveditz)

Comment 21

•

8 years ago

No, we'll have to re-evaluate. When would the second free happen? If it would happen in roughly the same action of loading the font (discovering it's bad and then not using it) then it would have a lower rating. If the second free doesn't come until much later (such as when the page is closed, or on the next GC) then you might be able to do some mischief and it could be "sec-high".

Flags: needinfo?(dveditz)

Keywords: sec-other

Comment 22

•

8 years ago

Neither Tyson nor Martin were able to reproduce in earlier comments, using a pre-fix version of the library, and presumably NOT using the -fno-lifetime-dse option because I don't see that used anywhere in the normal Firefox build files. Why? are there compiler differences that would protect Firefox-as-shipped from this potential bug?

Comment 23

•

8 years ago

It seems that the compiler optimization saved us from this bug, if my reading of the description of -fno-lifetime-dse on https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html is correct. We would not want to use -fno-lifetime-dse because it would expose us to such issues.

So I do not believe this would affect Firefox. It may affect other projects using the graphite code depending on how the library is built.

Comment 24

•

8 years ago

(In reply to Tyson Smith [:tsmith] from comment #23)
> It seems that the compiler optimization saved us from this bug, if my
> reading of the description of -fno-lifetime-dse on
> https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html is correct. We
> would not want to use -fno-lifetime-dse because it would expose us to such
> issues.

No, that option is what *prevents* the bug showing up; the default behavior (in a sufficiently new compiler version) is to apply the optimization, and that exposes the flaw.

(In reply to Daniel Veditz [:dveditz] from comment #21)
> No, we'll have to re-evaluate. When would the second free happen? If it
> would happen in roughly the same action of loading the font (discovering
> it's bad and then not using it) then it would have a lower rating.

Yes, it looks like it's happening then... this is all happening while trying to load a face, which is found to be invalid and discarded; the error comes from mishandling the destruction of objects when a table has been rejected, all happening at font-initialization time.

(In reply to Daniel Veditz [:dveditz] from comment #22)
> Neither Tyson nor Martin were able to reproduce in earlier comments, using a
> pre-fix version of the library, and presumably NOT using the
> -fno-lifetime-dse option because I don't see that used anywhere in the
> normal Firefox build files. Why? are there compiler differences that would
> protect Firefox-as-shipped from this potential bug?

From some comments I found, it sounded like the optimization that exposes this problem was introduced in GCC 4.9, so if we've been building with older compilers we wouldn't have run into it. (I guess clang probably does something similar, but I don't know in which versions...)

Updated

•

8 years ago

Group: core-security-release

Updated

•

8 years ago

Flags: sec-bounty?