Strengthen Harfbuzz code against outside use.

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);
 }