Fix race condition in reflow_layout.

There was a race condition between us dropping 'kept', relocking,
and using 'page'.

diff --git a/source/reflow/reflow-doc.c b/source/reflow/reflow-doc.c
index 06eb464cedfee22df461a987d096348c66974171..8f08e8af9e12896da45ba6273a3d3c756ce935df 100644 (file)
--- a/source/reflow/reflow-doc.c
+++ b/source/reflow/reflow-doc.c
@@ -185,6 +185,7 @@ static void reflow_layout(fz_context *ctx, reflow_document *doc, float w, float
 {
        reflow_page *page;
        fz_page *kept = NULL;
+       fz_page *dropme = NULL;
 
        if (doc->w == w && doc->h == h && doc->em == em)
                return;
@@ -193,24 +194,38 @@ static void reflow_layout(fz_context *ctx, reflow_document *doc, float w, float
        doc->em = em;
 
        fz_var(kept);
+       fz_var(dropme);
        fz_var(page);
        fz_try(ctx)
        {
+               /* We can only walk the page list while the alloc lock is taken, so gymnastics are required. */
+               /* Loop invariant: at any point where we might throw, kept != NULL iff we are unlocked. */
                fz_lock(ctx, FZ_LOCK_ALLOC);
                for (page = (reflow_page *)doc->base.open; page != NULL; page = (reflow_page *)page->base.next)
                {
+                       /* Keep an extra reference to the page so that no other thread can remove it. */
                        kept = fz_keep_page_locked(ctx, (fz_page *)page);
                        fz_unlock(ctx, FZ_LOCK_ALLOC);
+                       /* Drop any extra reference we might still have to a previous page. */
+                       fz_drop_page(ctx, dropme);
+                       dropme = NULL;
+                       /* Layout the page. */
                        fz_layout_document(ctx, page->html_doc, doc->w, 0, doc->em);
-                       fz_drop_page(ctx, kept);
+                       /* We can't drop kept here, because that would give us a race condition with
+                        * us taking the lock and hoping that 'page' would still be valid. So remember it
+                        * for dropping later. */
+                       dropme = kept;
                        kept = NULL;
                        fz_lock(ctx, FZ_LOCK_ALLOC);
                }
-               fz_unlock(ctx, FZ_LOCK_ALLOC);
+               /* unlock (and final drop of dropme) happens in the always. */
        }
        fz_always(ctx)
        {
+               if (kept == NULL)
+                       fz_unlock(ctx, FZ_LOCK_ALLOC);
                fz_drop_page(ctx, kept);
+               fz_drop_page(ctx, dropme);
        }
        fz_catch(ctx)
        {