Double free after codegen error

[clayton-shopify]

The following input demonstrates a double free bug:

a {
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b +=
  b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b += b
}

Result:

codegen error:193517-2.rb:34: too complex expression
mruby(2747,0x7fffd49793c0) malloc: *** error for object 0x7f84e4433a30: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
Abort trap: 6

This issue was first reported by https://hackerone.com/geeknik

@titanous later reported the same issue and diagnosed it as a double free of irep->filename:

The MRB_CATCH block in mrb_generate_code tries to avoid this by nulling the filename, but it doesn't take into account that ireps can be nested and so the filename in the nested irep gets freed even though it was not allocated for the irep.

He suggested the following patch:

From 44e50aa439acf092af28b3f5edd4ad3314c87106 Mon Sep 17 00:00:00 2001
From: Jonathan Rudenberg <jonathan@titanous.com>
Date: Fri, 23 Dec 2016 18:22:43 -0500
Subject: [PATCH 1/2] Fix double free of irep->filename after codegen error

---
 include/mruby/irep.h                  | 1 +
 mrbgems/mruby-compiler/core/codegen.c | 4 +---
 src/state.c                           | 5 ++++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/mruby/irep.h b/include/mruby/irep.h
index 8922f4b7..35ae2bba 100644
--- a/include/mruby/irep.h
+++ b/include/mruby/irep.h
@@ -39,6 +39,7 @@ typedef struct mrb_irep {
 
   struct mrb_locals *lv;
   /* debug info */
+  mrb_bool own_filename;
   const char *filename;
   uint16_t *lines;
   struct mrb_irep_debug_info* debug_info;
diff --git a/mrbgems/mruby-compiler/core/codegen.c b/mrbgems/mruby-compiler/core/codegen.c
index fc54a064..3bae67c6 100644
--- a/mrbgems/mruby-compiler/core/codegen.c
+++ b/mrbgems/mruby-compiler/core/codegen.c
@@ -2847,6 +2847,7 @@ scope_finish(codegen_scope *s)
     memcpy(fname, s->filename, fname_len);
     fname[fname_len] = '\0';
     irep->filename = fname;
+    irep->own_filename = TRUE;
   }
 
   irep->nlocals = s->nlocals;
@@ -2954,9 +2955,6 @@ mrb_generate_code(mrb_state *mrb, parser_state *p)
     return proc;
   }
   MRB_CATCH(&scope->jmp) {
-    if (scope->filename == scope->irep->filename) {
-      scope->irep->filename = NULL;
-    }
     mrb_irep_decref(mrb, scope->irep);
     mrb_pool_close(scope->mpool);
     return NULL;
diff --git a/src/state.c b/src/state.c
index 1259ac3a..11b71dd6 100644
--- a/src/state.c
+++ b/src/state.c
@@ -159,7 +159,9 @@ mrb_irep_free(mrb_state *mrb, mrb_irep *irep)
   }
   mrb_free(mrb, irep->reps);
   mrb_free(mrb, irep->lv);
-  mrb_free(mrb, (void *)irep->filename);
+  if (irep->own_filename) {
+    mrb_free(mrb, (void *)irep->filename);
+  }
   mrb_free(mrb, irep->lines);
   mrb_debug_info_free(mrb, irep->debug_info);
   mrb_free(mrb, irep);
@@ -261,6 +263,7 @@ mrb_add_irep(mrb_state *mrb)
   irep = (mrb_irep *)mrb_malloc(mrb, sizeof(mrb_irep));
   *irep = mrb_irep_zero;
   irep->refcnt = 1;
+  irep->own_filename = FALSE;
 
   return irep;
 }
-- 
2.11.0

@titanous also suggested this patch to fix a memory leak he noticed:

From ee8cfffe308f40aa31fb78a8b8f624b1439a7a7f Mon Sep 17 00:00:00 2001
From: Jonathan Rudenberg <jonathan@titanous.com>
Date: Fri, 23 Dec 2016 18:23:27 -0500
Subject: [PATCH 2/2] Fix leak of state->iseq during codegen error

---
 mrbgems/mruby-compiler/core/codegen.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mrbgems/mruby-compiler/core/codegen.c b/mrbgems/mruby-compiler/core/codegen.c
index 3bae67c6..b808cfb7 100644
--- a/mrbgems/mruby-compiler/core/codegen.c
+++ b/mrbgems/mruby-compiler/core/codegen.c
@@ -93,6 +93,7 @@ codegen_error(codegen_scope *s, const char *message)
   if (!s) return;
   while (s->prev) {
     codegen_scope *tmp = s->prev;
+    mrb_free(s->mrb, s->iseq);
     mrb_pool_close(s->mpool);
     s = tmp;
   }
-- 
2.11.0