diff options
author | Robin Watts <robin.watts@artifex.com> | 2016-03-28 12:48:07 +0100 |
---|---|---|
committer | Robin Watts <robin.watts@artifex.com> | 2016-03-28 12:49:13 +0100 |
commit | 588b69ccc185f9c108228c298426e42aac2d2967 (patch) | |
tree | 3342de194ab3bf6c37076fd78375a8c2cf574fde | |
parent | 5a72b781cb1a44951c60a2a91c8de6e0b97f03f5 (diff) | |
download | mupdf-588b69ccc185f9c108228c298426e42aac2d2967.tar.xz |
Strengthen Harfbuzz code against outside use.
-rw-r--r-- | source/fitz/harfbuzz.c | 74 |
1 files changed, 51 insertions, 23 deletions
diff --git a/source/fitz/harfbuzz.c b/source/fitz/harfbuzz.c index 60d75469..30b82c5d 100644 --- a/source/fitz/harfbuzz.c +++ b/source/fitz/harfbuzz.c @@ -7,6 +7,49 @@ #include "hb.h" +/* Harfbuzz has some major design flaws. + * + * It is utterly thread unsafe. It uses statics to hold + * structures in, meaning that if it is ever called from + * more than one thread at a time, access to such + * structures can race. We work around this by imposing + * a lock on our calls to harfbuzz (we reuse the freetype + * lock). + * + * This does not protect us against the possibility of + * other people calling harfbuzz; for instance, if we + * link MuPDF into an app that either calls harfbuzz + * itself, or uses another library that calls harfbuzz, + * there is no guarantee that that library will take + * the same lock while calling harfbuzz. This leaves + * us open to the possibility of crashes. The only + * way around this would be to use completely separate + * harfbuzz instances. + * + * In order to ensure that allocations throughout mupdf + * are done consistently, we get harfbuff to call our + * own hb_malloc/realloc/calloc/free functions that + * call down to fz_malloc/realloc/calloc/free. These + * require context variables, so we get our hb_lock + * and unlock to set these. Any attempt to call through + * without setting these is detected. + * + * It is therefore vital that any fz_lock/fz_unlock + * handlers are shared between all the fz_contexts in + * use at a time. + * + * Finally, harfbuzz stores various malloced structures + * in statics. These can either be left to leak on + * closedown, or (if HAVE_ATEXIT is defined), they will + * be freed using various 'atexit' callbacks. Unfortunately + * this won't reset the fz_context value, so the allocators + * can't free them correctly. + * + * Consequently, we leave them to leak, and warn Memento + * about this. + */ + + /* Potentially we can write different versions * of get_context and set_context for different * threading systems. @@ -16,11 +59,8 @@ * different threads. The only way that can happen * is when one of those other threads is someone * outside MuPDF calling harfbuzz while MuPDF - * is running. - * - * If this is actually a problem, then we can - * reimplement set_context/get_context using - * Thread Local Storage. + * is running. This will cause us such huge + * problems that for now, we'll just forbid it. */ static fz_context *hb_secret = NULL; @@ -53,10 +93,7 @@ void *hb_malloc(size_t size) { fz_context *ctx = get_context(); - /* Should never happen, but possibly someone else - * is calling our version of the library. */ - if (ctx == NULL) - return malloc(size); + assert(ctx != NULL); return fz_malloc_no_throw(ctx, (unsigned int)size); } @@ -65,10 +102,7 @@ void *hb_calloc(size_t n, size_t size) { fz_context *ctx = get_context(); - /* Should never happen, but possibly someone else - * is calling our version of the library. */ - if (ctx == NULL) - return calloc(n, size); + assert(ctx != NULL); return fz_calloc_no_throw(ctx, (unsigned int)n, (unsigned int)size); } @@ -77,10 +111,7 @@ void *hb_realloc(void *ptr, size_t size) { fz_context *ctx = get_context(); - /* Should never happen, but possibly someone else - * is calling our version of the library. */ - if (ctx == NULL) - return realloc(ptr, size); + assert(ctx != NULL); return fz_resize_array_no_throw(ctx, ptr, (unsigned int)1, (unsigned int)size); } @@ -89,10 +120,7 @@ void hb_free(void *ptr) { fz_context *ctx = get_context(); - /* Should never happen, but possibly someone else - * is calling our version of the library. */ - if (ctx == NULL) - free(ptr); - else - fz_free(ctx, ptr); + assert(ctx != NULL); + + fz_free(ctx, ptr); } |