From 85fcbde22fe1068e9ef2d73a57d8a813e24eef12 Mon Sep 17 00:00:00 2001 From: George Koehler Date: Mon, 12 Mar 2018 20:58:31 -0400 Subject: [PATCH] Check LOI expressions to prevent a read after free. CS eliminates outer expressions before inner ones, as `x * y * z` before `x * y`. It does this by reversing the order of expressions in the code. This almost always works, but it sometimes doesn't work if a STI changes the value number of a LOI. In code like `expr1 LOI expr2 STI expr2 LOI`, CS might eliminate the inner `expr2` before the outer `expr2 LOI`. This caused a read after free because the occurrence of `expr2 LOI` pointed to the eliminated lines of `expr2`. This bug went unnoticed until my recent changes caused CS to crash with a double free. I did not get the crash in OpenBSD, but I saw the crash in Travis, then David Given reproduced the crash in Linux. See the discussion in https://github.com/davidgiven/ack/pull/73 --- util/ego/cs/cs_elim.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/util/ego/cs/cs_elim.c b/util/ego/cs/cs_elim.c index 7dce0df09..b83371416 100644 --- a/util/ego/cs/cs_elim.c +++ b/util/ego/cs/cs_elim.c @@ -142,8 +142,9 @@ STATIC void replace(occur_p ocp, offset tmp, avail_p avp) /* Replace the lines in the occurrence in ocp by a load of the * temporary with offset tmp. */ - register line_p lol, first, last; - register int instr; + avail_p ravp; + line_p lol, first, last; + int instr; assert(avp->av_size == ws || avp->av_size == 2*ws); @@ -176,6 +177,33 @@ STATIC void replace(occur_p ocp, offset tmp, avail_p avp) break; } + /* Some occurrence rocp of an expression before avp might have + * rocp->oc_lfirst == first. If so, then we must set + * rocp->oc_lfirst = lol before we throw away first. + * + * This is almost not possible, but it can happen in code with + * expr1 LOI expr2 STI expr2 LOI, where the STI causes both + * LOIs to have the same value number. Then the first LOI + * might come before the first expr2, so we might replace + * expr2 before we replace expr2 LOI. Then the occurrence of + * expr2 LOI must not point to the eliminated lines of expr2. + */ + for (ravp = avp->av_before; ravp != (avail_p) 0; + ravp = ravp->av_before) { + /* We only check LOI expressions. */ + if (ravp->av_instr == op_loi) { + occur_p rocp; + Lindex i; + + for (i = Lfirst(ravp->av_occurs); i != (Lindex) 0; + i = Lnext(i, ravp->av_occurs)) { + rocp = occ_elem(i); + if (rocp->oc_lfirst == first) + rocp->oc_lfirst = lol; + } + } + } + /* Throw away the by now useless lines. */ remove_lines(first, last); }