Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Must not crash on null family_name #124

Closed
springmeyer opened this issue Nov 22, 2016 · 2 comments
Closed

Must not crash on null family_name #124

springmeyer opened this issue Nov 22, 2016 · 2 comments

Comments

@springmeyer
Copy link
Contributor

springmeyer commented Nov 22, 2016

Context

Saw a crash like:

12/22/2016 19:16:39.052 +0000	terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_S_construct null not valid
/usr/local/bin/logbt: line 112:    96 Aborted                 (core dumped) $*
12/22/2016 19:16:36.906 +0000	#11 0x00007eff9c8c037d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
12/22/2016 19:16:36.882 +0000	#10 0x00007eff9cb93184 in start_thread (arg=0x7eff8b7fe700) at pthread_create.c:312
12/22/2016 19:16:36.881 +0000	#9  0x0000000000f9f8b9 in uv__thread_start (arg=<optimized out>) at ../deps/uv/src/unix/thread.c:49
12/22/2016 19:16:36.881 +0000	#8  0x0000000000f90c81 in worker (arg=arg@entry=0x0) at ../deps/uv/src/threadpool.c:95
12/22/2016 19:16:36.881 +0000	#7  0x00007eff9838bc39 in node_fontnik::LoadAsync(uv_work_s*) () from .../node_modules/@mapbox/mapbox-core-fonts/node_modules/fontmachine/node_modules/fontnik/lib/fontnik.node
12/22/2016 19:16:36.880 +0000	#6  0x00007eff983a2278 in void std::vector<node_fontnik::FaceMetadata, std::allocator<node_fontnik::FaceMetadata> >::_M_emplace_back_aux<char*&, char*&, std::vector<int, std::allocator<int> > >(char*&, char*&, std::vector<int, std::allocator<int> >&&) () from .../node_modules/@mapbox/mapbox-core-fonts/node_modules/fontmachine/node_modules/fontnik/lib/fontnik.node
12/22/2016 19:16:36.879 +0000	#5  0x00007eff9d32fe46 in __cxa_rethrow () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
12/22/2016 19:16:36.879 +0000	#4  0x00007eff9d32fbe1 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6

After investigating, it is happening because:

  1. A null ptr is being passed to std::string
  2. The C++ exception is thrown but we are missing a try/cache in loadAsync so it goes unhandled and ends up aborting the entire process

I confirmed that this error can come from passing a NULL string, and when unhandled will abort:

$ echo "#include <iostream>" > test.cpp
$ echo "int main() { std::string s(NULL); }" >> test.cpp
$ g++ -o run-test test.cpp
$ ./run-test 
terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_S_construct null not valid
Aborted (core dumped)
echo $?
134

Fixing the problem

We need to:

  • try/catch to ensure problems like this are returned to JS as errors rather than aborts
  • Prevent NULL strings to avoid potential crashes on OS X and avoid the underlying error ever happening on Linux

The remaining question is where in the loadAsync could a NULL be sent to std::string. I dug into the freetype docs and found it is possible for fonts to have a NULL family name per the Can be NULL in:

The face's family name. This is an ASCII string, usually in English, that describes the typeface's family (like ‘Times New Roman’, ‘Bodoni’, ‘Garamond’, etc). This is a least common denominator used to list fonts. Some formats (TrueType & OpenType) provide localized and Unicode versions of this string. Applications should use the format specific interface to access them. Can be NULL (e.g., in fonts embedded in a PDF file).

refs https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT_FaceRec

We are currently not catching this case, which leads to a crash like this:

(lldb) bt
* thread #7: tid = 0xeddbc, 0x00007fff927e2132 libsystem_c.dylib`strlen + 18, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00007fff927e2132 libsystem_c.dylib`strlen + 18
    frame #1: 0x000000010412bdd5 fontnik.node`std::__1::char_traits<char>::length(char const*) + 21
    frame #2: 0x00000001040062f2 fontnik.node`node_fontnik::RangeAsync(uv_work_s*) [inlined] std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string(this="", __s=0x0000000000000000) + 87 at string:2021
    frame #3: 0x000000010400629b fontnik.node`node_fontnik::RangeAsync(uv_work_s*) [inlined] std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string(this="", __s=0x0000000000000000) + 21 at string:2019
    frame #4: 0x0000000104006286 fontnik.node`node_fontnik::RangeAsync(req=0x00000001020006d0) + 5014 at glyphs.cpp:295
    frame #5: 0x000000010078342e node`worker + 90
    frame #6: 0x000000010078f63c node`uv__thread_start + 25
    frame #7: 0x00007fff9da6d99d libsystem_pthread.dylib`_pthread_body + 131
    frame #8: 0x00007fff9da6d91a libsystem_pthread.dylib`_pthread_start + 168
    frame #9: 0x00007fff9da6b351 libsystem_pthread.dylib`thread_start + 13

While fonts without a family name are rare I was able to find one from the harfbuzz testsuite: https://github.com/behdad/harfbuzz/blob/7793aad946e09b53523b30d57de85abd1d15f8b6/test/shaping/fonts/sha1sum/1c2c3fc37b2d4c3cb2ef726c6cdaaabd4b7f3eb9.ttf

//cc @mikemorris @xrwang @ianshward

springmeyer pushed a commit that referenced this issue Nov 23, 2016
springmeyer pushed a commit that referenced this issue Nov 23, 2016
@springmeyer
Copy link
Contributor Author

Note: this results in a SIGABRT (exit code 134) on linux and a SEGSEGV (exit code 139) on OSX.

Linux:

# load font with no family name
terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_S_construct null not valid
Aborted (core dumped)
ubuntu@ip-10-22-34-158:~/node-fontnik$ echo $?
134

OS X:

# load font with no family name
Segmentation fault: 11
~/projects/node-fontnik[null-family-testcase]$ echo $?
139

@springmeyer
Copy link
Contributor Author

Next action is to merge #128 which fixes this issue. Closing this one and will track at #128.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant