[DTrace-devel] [PATCH 20/47] Fix double-free of dtp->dt_strtab

[Kris Van Hees]

The consolidated string table that is used to populate the strtab BPF
map when we prepare to load and execute BPF programs was being free'd
twice (first when the DIFOs are destroyed and second when we are closing
the DTrace consumer handle).  The reason for this is that dtp->dt_strtab
was being constructed anew for each DIFO, and then assigned to the
dtdo_strtab member for the DIFO as well.  So, the last DIFO that got
assembled would have a reference to the same string table as the DTrace
consumer handle.

When that DIFO got destroyed (dt_difo_free()), the string table would be
free'd as well.  Then, in dt_close() we would try to free dtp->dt_strtab
which arready got free'd.

We now (correctly) generate string table data for each DIFO, and assign
it to dtp->dt_strtab (size is stored in dtp->strlen).  This is sufficient
because we only care about the last one that get generated (which by
design contains all strings from all prior DIFOs).  We know that the
string table will be free'd in dt_difo_free() so we ensure that the
dtp->dt_strtab and dtp->dt_strlen are cleared when the DIFO referencing
the same string table as the consumer handle is free'd, and we refrain
from freeing dtp->dt_strtab altogether.

Orabug: 31220517
Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
---
 libdtrace/dt_as.c   | 32 ++++++++++++++++++++++++--------
 libdtrace/dt_open.c |  2 --
 libdtrace/dt_subr.c | 13 ++++++++++++-
 3 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/libdtrace/dt_as.c b/libdtrace/dt_as.c
index 2ddbc886..42c8228a 100644
--- a/libdtrace/dt_as.c
+++ b/libdtrace/dt_as.c
@@ -535,20 +535,36 @@ fail:
 
 	/*
 	 * Allocate memory for the compiled string table and then copy the
-	 * chunks from the string table into the final string buffer.
+	 * chunks from the string table into the final string buffer for the
+	 * DIFO we are constructing.
+	 *
+	 * We keep the string table around (dtp->dt_csstab) because further
+	 * compilations may add more strings.  We will load a consolidated
+	 * compiled string table into a strtab BPF map so it can be used ny
+	 * all BPF programs we will be loading.  Since each DIFO will have a
+	 * string table that comprises the strings of all DIFOs before it along
+	 * with any new ones, we simply assign the latest (and most complete)
+	 * string table to dtp->dt_strtab (and the size as dtp->dt_strlen), so
+	 * that when we are ready to load and execute programs, we know we have
+	 * the latest and greatest string table to work with.
+	 *
+	 * Note that this means that we should *not* free dtp->dt_strtab later
+	 * because it will be free'd when the DIFO is destroyed.
 	 */
-	if ((n = dt_strtab_size(dtp->dt_ccstab)) != 0) {
-		if ((dtp->dt_strtab = dt_alloc(dtp, n)) == NULL)
+	dp->dtdo_strlen = dt_strtab_size(dtp->dt_ccstab);
+	if (dp->dtdo_strlen > 0) {
+		dp->dtdo_strtab = dt_zalloc(dtp, dp->dtdo_strlen);
+		if (dp->dtdo_strtab == NULL)
 			longjmp(pcb->pcb_jmpbuf, EDT_NOMEM);
 
 		dt_strtab_write(dtp->dt_ccstab,
 				(dt_strtab_write_f *)dt_strtab_copystr,
-				dtp->dt_strtab);
-		dtp->dt_strlen = (uint32_t)n;
+				dp->dtdo_strtab);
 
-		dp->dtdo_strtab = dtp->dt_strtab;
-		dp->dtdo_strlen = dtp->dt_strlen;
-	}
+		dtp->dt_strtab = dp->dtdo_strtab;
+		dtp->dt_strlen = dp->dtdo_strlen;
+	} else
+		dp->dtdo_strtab = NULL;
 
 	/*
 	 * Fill in the DIFO return type from the type associated with the
diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
index 42e15bac..e8c78492 100644
--- a/libdtrace/dt_open.c
+++ b/libdtrace/dt_open.c
@@ -1160,8 +1160,6 @@ dtrace_close(dtrace_hdl_t *dtp)
 		dt_ident_destroy(idp);
 	}
 
-	if (dtp->dt_strtab != NULL)
-		dt_free(dtp, dtp->dt_strtab);
 	if (dtp->dt_ccstab != NULL)
 		dt_strtab_destroy(dtp->dt_ccstab);
 	if (dtp->dt_macros != NULL)
diff --git a/libdtrace/dt_subr.c b/libdtrace/dt_subr.c
index 14824578..337827f4 100644
--- a/libdtrace/dt_subr.c
+++ b/libdtrace/dt_subr.c
@@ -1,6 +1,6 @@
 /*
  * Oracle Linux DTrace.
- * Copyright (c) 2010, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
  * Licensed under the Universal Permissive License v 1.0 as shown at
  * http://oss.oracle.com/licenses/upl.
  */
@@ -726,7 +726,18 @@ dt_difo_free(dtrace_hdl_t *dtp, dtrace_difo_t *dp)
 	if (dp == NULL)
 		return; /* simplify caller code */
 
+	/*
+	 * If we are destroying the DIFO that holds the final consolidated
+	 * string table, clear the dt_strtab and dt_strlen members in the
+	 * consumer handle.
+	 */
+	if (dtp->dt_strtab == dp->dtdo_strtab) {
+		dtp->dt_strtab = NULL;
+		dtp->dt_strlen = 0;
+	}
+
 	dt_free(dtp, dp->dtdo_buf);
+	dt_free(dtp, dp->dtdo_strtab);
 	dt_free(dtp, dp->dtdo_vartab);
 	dt_free(dtp, dp->dtdo_kreltab);
 	dt_free(dtp, dp->dtdo_ureltab);
-- 
2.26.0