use-after-free bug on container timeout
Opened this issue · 0 comments
Hello,
I think I have found an "use after free" bug in the map expiration timers.
I am currently implementing timeout on Spicy containers https://github.com/FrozenCaribou/hilti/tree/spicy_container_timeout
Here my pac2 example :
module Test;
import BinPAC;
import Bro;
global m: map<bytes, bytes >;
m.timeout(BinPAC::ExpireStrategy::Create, interval(60.0));
export type Foo = unit{
t : bytes &length=1;
key : bytes &length=4;
value : bytes &eod;
on %done{
if (self.t == b"a"){
m[self.key] = self.value;
}
print m;
}
};
The evt file :
grammar ./test.pac2;
protocol analyzer pac2::Test over UDP:
parse with Test::Foo,
port 1337/udp;
The equivalent Hilti code of the problem:
m = new map<string, string>
map.timeout m Hilti::ExpireStrategy::Create interval(5.0)
map.insert m "A-0" "test"
timer_mgr.advance time(6.0) t
Note that map.timeout uses the context global time mgr.
When map.insert is executed, we execute this following code:
https://github.com/rsmmr/hilti/blob/master/libhilti/map_set.c#L418-L422
__hlt_map_timer_cookie cookie = { m, keytmp };
kh_value(m, i).timer = __hlt_timer_new_map(cookie, excpt, ctx);
hlt_time t = hlt_timer_mgr_current(m->tmgr, excpt, ctx) + m->timeout;
hlt_timer_mgr_schedule(m->tmgr, t, kh_value(m, i).timer, excpt, ctx);
GC_DTOR(kh_value(m, i).timer, hlt_timer, ctx); // Not memory-managed on our end.
On map entry insertion, a timer is created and its ref_cnt = 1 (set by hlt_timer_mgr_schedule
) but after the GC_DTOR
the timer is unreferenced and ref_cnt = 0.
There is no visible impact while the memory is not flushed ... ( with __hlt_memory_nullbuffer_flush
)
In the expire unit test ( https://github.com/rsmmr/hilti/blob/master/tests/hilti/map/expire.hlt ) we cannot see this issue because there is no flush of the nullbuffer
buffer. Flush are done with safepoint which are used in Spicy parsers.
If a safepoint is created, the __hlt_memory_nullbuffer_flush
function is executed, the previous address of the timer will be free (because its ref=0) and the value in the ram memory can change with other malloc/calloc executed by the application.
Then the timer manager checks timers and fires them, __hlt_timer_fire
( https://github.com/rsmmr/hilti/blob/master/libhilti/timer.c#L67 ) deals with a bad timers (data overwritten by others malloc) and produces unpredictable behavior as a segmentation fault.
I created a patch and removed in the hlt_map_insert
(and for set/list/vector) the following line:
GC_DTOR(kh_value(m, i).timer, hlt_timer, ctx); // Not memory-managed on our end.
I think it is ok because timers are also deleted when it expires, cancels or is deleted (dtor) but I am not totally sure. Here the commit : FrozenCaribou@a6f7434
This works well but there is another bug on hlt_execution_context_delete
that appears when I stop Bro with Ctrl + C (Unix signal 2) whereas there are still expiration pending timers, it is during the hlt_execution_context_delete
(__hlt_memory_nullbuffer_delete
)
I have a bad access memory in priority_queue_remove
because *mgr->timers->d
is null (crashes at when getpri(q->d[posn] = 0x0 )
)
I fixed the bug by checking this value before callingpriority_queue_remove
. See commit : FrozenCaribou@f1ba048
I suspect that that global timer manager is destroyed before the map and so priority_queue_remove
is executed after priority_queue_free
which performs a hlt_free
on all elements.
I do not have a good enough vision of memory management in Hilti, but from comments : timers are "not memory-managed on our end." in container timeout. There seems to be manage directly in pqueue (there are removed on priority_queue_free
) so I am wondering why there are present and referenced in the nullbuffer ?
I found the commit relative to this memory management : 7b407cf
I also notice some obj ref can underflow, I will take a look on that.