search

bnagy / gapstone

gapstone is a Go binding for the capstone disassembly library
Other
151 stars 43 forks source link

use after free in engine.Disasm #9

Closed StalkR closed 10 years ago

StalkR commented 10 years ago

Hi Ben,

I was using a debug build of capstone+gapstone which poisons bytes when they're freed, and it revealed a subtle use-after-free issue in engine.Disasm. Because of the defer cs_free of insn, the elements pointed by the returned slice will be freed when the function returns, so when the caller tries to access the disassembly, it points to freed memory. It was revealed by https://github.com/bnagy/gapstone/blob/master/x86_decomposer_test.go#L20 which in a normal build and test run shows x86.SPEC: Prefix:0x00 0x00 0x00 0x00 Opcode:0x8d 0x00 0x00 0x00 but with a debug build poisoning bytes when it's freed with 0xcd, it shows: Prefix:0xcd 0xcd 0xcd 0xcd Opcode:0xcd 0xcd 0xcd 0xcd

Attached is a patch that removes the cs_free I'm talking about, but don't merge it: it creates a memory leak, I checked with a program doing many engine.Disasm. So I'm not sure how you want to fix this: we could provide a method for callers to free this, but that's not elegant; it could be better to copy this slice backed by C elements into a Go slice that would then be managed by the garbage collector.

Thoughts?

Cheers

bnagy commented 10 years ago

"it could be better to copy this slice backed by C elements into a Go slice that would then be managed by the garbage collector."

That's exactly what I thought I was doing - the reflected slice is passed to the _decomposer.go files, but the return value is a new slice that should be brand new pure-go instructions. Are you able to test the other architectures to see if you get the same issues? I can't look at this properly for a couple of days...

It might be good if you'd open an issue for this instead of working in this PR.

Thanks!

StalkR commented 10 years ago

Ok closing this, I'll open an issue and test other architectures.