fix double-free/use-after-free of pointers in struct e1inp_line
Ensure that pointers in cloned e1inp_lines point to valid memory.
Some members of struct e1inp_line can simply be deep-copied.
Use talloc reference counting for pointers to objects which may
be shared between clones (driver-private state and counters).
Prevents double-free bugs, e.g. when multiple links referring
to the same line are closed.
Also, do not forget to unlink struct e1inp_line's counter group from
the counter list. Fixes use-after-free in rate_ctr_timer_cb() during
osmo-bts shutdown.
Change-Id: I9f4724b4a5a064801591e9acf4f2fd1db006d082
Related: OS#3011
Related: OS#3137
diff --git a/src/e1_input.c b/src/e1_input.c
index 29ba440..11949a1 100644
--- a/src/e1_input.c
+++ b/src/e1_input.c
@@ -394,6 +394,25 @@
return NULL;
memcpy(clone, line, sizeof(struct e1inp_line));
+
+ if (line->name) {
+ clone->name = talloc_strdup(clone, line->name);
+ OSMO_ASSERT(clone->name);
+ }
+ if (line->sock_path) {
+ clone->sock_path = talloc_strdup(clone, line->sock_path);
+ OSMO_ASSERT(clone->sock_path);
+ }
+
+ /*
+ * Rate counters and driver data are shared between clones. These are pointers
+ * to dynamic memory so we use reference counting to avoid a double-free (see OS#3137).
+ */
+ OSMO_ASSERT(line->rate_ctr);
+ clone->rate_ctr = talloc_reference(clone, line->rate_ctr);
+ if (line->driver_data)
+ clone->driver_data = talloc_reference(clone, line->driver_data);
+
clone->refcnt = 1;
return clone;
}
@@ -406,8 +425,28 @@
void e1inp_line_put(struct e1inp_line *line)
{
line->refcnt--;
- if (line->refcnt == 0)
+ if (line->refcnt == 0) {
+ /* Remove our counter group from libosmocore's global counter
+ * list if we are freeing the last remaining talloc context.
+ * Otherwise we get a use-after-free when libosmocore's timer
+ * ticks again and attempts to update these counters (OS#3011).
+ *
+ * Note that talloc internally counts "secondary" references
+ * _in addition to_ the initial allocation context, so yes,
+ * we must check for *zero* remaining secondary contexts here. */
+ if (talloc_reference_count(line->rate_ctr) == 0) {
+ rate_ctr_group_free(line->rate_ctr);
+ } else {
+ /* We are not freeing the last talloc context.
+ * Instead of calling talloc_free(), unlink this 'line' pointer
+ * which serves as one of several talloc contexts for the rate
+ * counters and driver private state. */
+ talloc_unlink(line, line->rate_ctr);
+ if (line->driver_data)
+ talloc_unlink(line, line->driver_data);
+ }
talloc_free(line);
+ }
}
void