use-after-free bug on container timeout

[FrozenCaribou]

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 : https://github.com/FrozenCaribou/hilti/commit/a6f74340b68e1e5e9875ef282524c899b4727103

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 : https://github.com/FrozenCaribou/hilti/commit/f1ba0486a97e482dcf4d8c7d54bdfb01a4670197

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 : https://github.com/rsmmr/hilti/commit/7b407cf42eebb662dc1058ce796a9f3dc4363091

I also notice some obj ref can underflow, I will take a look on that.