MPR#7609: use-after-free with ocamldebug and Pervasives.flush_all (alternative solution)MPR7609

Rather than play with refcounts as in commits 796c796 and 43ed007, this commit adds a flag CHANNEL_FLAG_MANAGED_BY_GC that governs whether the struct channel can be destroyed by GC finalization.

Internal channels opened by the runtime system for its own purposes do not have this flag and will never be finalized, even if they end up in the Caml heap as a consequence of Pervasives.flush_all.

3 files changed, 9 insertions, 5 deletions

diff --git a/byterun/caml/io.h b/byterun/caml/io.h
index f388bd9fb4..87de679e53 100644
--- a/byterun/caml/io.h
+++ b/byterun/caml/io.h
@@ -55,8 +55,9 @@ struct channel {
 enum {
   CHANNEL_FLAG_FROM_SOCKET = 1,  /* For Windows */
 #if defined(NATIVE_CODE) && defined(WITH_SPACETIME)
-  CHANNEL_FLAG_BLOCKING_WRITE = 2,
+  CHANNEL_FLAG_BLOCKING_WRITE = 2, /* Don't release master lock when writing */
 #endif
+  CHANNEL_FLAG_MANAGED_BY_GC = 4,  /* Free and close using GC finalization */
 };
 
 /* For an output channel:
diff --git a/byterun/debugger.c b/byterun/debugger.c
index dfcc6c6acd..2dccb87033 100644
--- a/byterun/debugger.c
+++ b/byterun/debugger.c
@@ -131,7 +131,6 @@ static void open_connection(void)
 #endif
   dbg_in = caml_open_descriptor_in(dbg_socket);
   dbg_out = caml_open_descriptor_out(dbg_socket);
-  dbg_out->refcount++; /* prevent deallocation at finalization, MPR#7609 */
   if (!caml_debugger_in_use) caml_putword(dbg_out, -1); /* first connection */
 #ifdef _WIN32
   caml_putword(dbg_out, _getpid());
@@ -144,7 +143,6 @@ static void open_connection(void)
 static void close_connection(void)
 {
   caml_close_channel(dbg_in);
-  dbg_out->refcount--;
   caml_close_channel(dbg_out);
   dbg_socket = -1;              /* was closed by caml_close_channel */
 }
diff --git a/byterun/io.c b/byterun/io.c
index 2cd6816516..3d9560198a 100644
--- a/byterun/io.c
+++ b/byterun/io.c
@@ -394,6 +394,7 @@ CAMLexport intnat caml_input_scan_line(struct channel *channel)
 CAMLexport void caml_finalize_channel(value vchan)
 {
   struct channel * chan = Channel(vchan);
+  if ((chan->flags & CHANNEL_FLAG_MANAGED_BY_GC) == 0) return;
   if (--chan->refcount > 0) return;
   if (caml_channel_mutex_free != NULL) (*caml_channel_mutex_free)(chan);
 
@@ -461,12 +462,16 @@ CAMLexport value caml_alloc_channel(struct channel *chan)
 
 CAMLprim value caml_ml_open_descriptor_in(value fd)
 {
-  return caml_alloc_channel(caml_open_descriptor_in(Int_val(fd)));
+  struct channel * chan = caml_open_descriptor_in(Int_val(fd));
+  chan->flags |= CHANNEL_FLAG_MANAGED_BY_GC;
+  return caml_alloc_channel(chan);
 }
 
 CAMLprim value caml_ml_open_descriptor_out(value fd)
 {
-  return caml_alloc_channel(caml_open_descriptor_out(Int_val(fd)));
+  struct channel * chan = caml_open_descriptor_out(Int_val(fd));
+  chan->flags |= CHANNEL_FLAG_MANAGED_BY_GC;
+  return caml_alloc_channel(chan);
 }
 
 CAMLprim value caml_ml_set_channel_name(value vchannel, value vname)