search

microsoft / rego-cpp

A C++ interpreter for the OPA policy language Rego
https://microsoft.github.io/rego-cpp/
MIT License
33 stars 10 forks source link

rego-cpp inconsistently replaces malloc #128

Closed davidchisnall closed 5 months ago

davidchisnall commented 8 months ago

I suspect this may be a Trieste problem inherited by rego-cpp.

The headers appear to substitute snmalloc's malloc and operator new. This causes problems when system-provided shared libraries do not and borrow buffers. For example, __cxa_demangle takes a malloc'd buffer and expects to be able to call realloc on it. If the caller has included the rego-cpp headers then the malloc'd buffer does not come from the system malloc and so the realloc call corrupts the heap.

mjp41 commented 8 months ago

@davidchisnall I had a look, and Trieste seems to be including snmallocshim-static, which I thought would be correct? Could you post me a repro, so I can investigate further?

davidchisnall commented 8 months ago

https://github.com/CHERIoT-Platform/cheriot-audit

Linking on macOS, we call __cxa_demangle, which is provided by /usr/lib/libc++.1.dylib. This gets its malloc symbol from /usr/lib/libSystem.B.dylib.

Running this test:

https://github.com/CHERIoT-Platform/cheriot-audit/blob/main/Tests/demangle_compartment_call.query

Crashes with a malloc error if this malloc is too small:

https://github.com/CHERIoT-Platform/cheriot-audit/blob/8192fd3857d93e20bbe6bffbe177a6c942c9e780/audit.cc#L132

I've added a hack to make it try to guess an adequate size.

In general, a library replacing malloc for an entire program is not a good idea. It would be nice if Trieste had an option to use snmalloc, but didn't do it unconditionally, then we could turn it off.

davidchisnall commented 8 months ago

(Note: for that specific case, I can pass nullptr to force the callee to allocate, but then I get back a malloc'd pointer and I need to free it with the system malloc. I'm not sure what happens if I pass it to snmalloc's free)

mjp41 commented 8 months ago

(Note: for that specific case, I can pass nullptr to force the callee to allocate, but then I get back a malloc'd pointer and I need to free it with the system malloc. I'm not sure what happens if I pass it to snmalloc's free)

Another crash.

mjp41 commented 8 months ago

https://github.com/CHERIoT-Platform/cheriot-audit

Linking on macOS, we call __cxa_demangle, which is provided by /usr/lib/libc++.1.dylib. This gets its malloc symbol from /usr/lib/libSystem.B.dylib.

Running this test:

https://github.com/CHERIoT-Platform/cheriot-audit/blob/main/Tests/demangle_compartment_call.query

Crashes with a malloc error if this malloc is too small:

https://github.com/CHERIoT-Platform/cheriot-audit/blob/8192fd3857d93e20bbe6bffbe177a6c942c9e780/audit.cc#L132

I've added a hack to make it try to guess an adequate size.

In general, a library replacing malloc for an entire program is not a good idea. It would be nice if Trieste had an option to use snmalloc, but didn't do it unconditionally, then we could turn it off.

Probably best to have it off by default, and then we can turn it on where we know it works. It is already off for Windows as that doesn't work.

matajoh commented 5 months ago

I've updated to the latest version of Trieste, which has an explicit flag to disable snmalloc new override, which I believes resolves this issue.