oils-for-unix / oils

Oils is our upgrade path from bash to a better language and runtime. It's also for Python and JavaScript users who avoid shell!
http://www.oilshell.org/
Other
2.85k stars 158 forks source link

asdl/format.py has heap-allocated BufWriter, which leak #1317

Closed andychu closed 1 year ago

andychu commented 2 years ago

Because the internal data_ pointer is malloc'd and we never call BufWriter::reset() which calls free()

andychu commented 2 years ago

This is exposed by mycpp/examples/parse.py

andychu commented 2 years ago

This one is good because it has a test with a simple verification

andychu commented 2 years ago
- Sweep -> free() 
- AllocStr(2 * old_size_)
- memcpy() 
andychu commented 2 years ago

@CoffeeTableEspresso

got rid of some leaks by explicitly calling reset(), also needed to change mycpp/mylib.pyi

Is this right architecturally?

andychu commented 2 years ago

@CoffeeTableEspresso I think I figured out a principled solution in the shower (of course)

It's not valid under Cheney though (because you can't really realloc with a bump allocator, although I guess you can simulate it ?)


Also to fix the quadratic problem it should be

realloc(10), realloc(20), realloc(40)  realloc(80) -- doubling

not

realloc(10), realloc(20), realloc(30)  realloc(40) realloc(50) ...

Basic knowledge that I've never really needed to use :)

andychu commented 2 years ago

Or actually an easier option is to make BufWriter itself like Str -- it uses the "flexible array" pattern like char buf_[1] ? And then placement new?

Although this could complicate generated code ... It couldn't use Alloc<BufWriter>, it would have to use a custom NewBufWriter

andychu commented 2 years ago

Also we zero'd for Cheney and string NUL termination of C strings

But this is slightly harder with realloc(), so maybe we can get away with just doing NUL termination and not zeroing the whole buffer

https://stackoverflow.com/questions/2141277/how-to-zero-out-new-memory-after-realloc

andychu commented 1 year ago

@CoffeeTableEspresso fixed this!