search

mirage / bechamel

Agnostic benchmark in OCaml (proof-of-concept)
MIT License
44 stars 15 forks source link

Fix double free in Test.uniq #47

Closed edwintorok closed 9 months ago

edwintorok commented 9 months ago

Draft pull request because I'd also like to add some unit tests that exercise the basic functionality of bechamel and check for bugs like double free.

The 'allocate1' function was unused for Test.Uniq, except for the KDE calculation where it was called with '1' always (in which case 'allocate0' would be better suited).

So do not allocate anything in 'allocate1' for Test.Uniq, just return an empty array: this avoids the double free where allocate gets called multiple times, but the allocation function returns the same preallocated value, which free would attempt to free multiple times. Previously this was happening:

v  = allocate 1
allocate1 = always v
free1 = free v 
allocate1 = always v
free1 = free v (DOUBLE FREE!)

Also change allocate0 for Test.Multiple which will now get called at least twice if KDE is used, avoid the double free on that for similar reasons.

edwintorok commented 9 months ago

(unfortunately OCaml doesn't have yet a feature to detect double free at compile time, although I read about some work to add modality which would allow to detect this at compile time. Meanwhile having some unit tests should help catch at least the obvious double frees)

dinosaure commented 9 months ago

Thanks for this PR, I will try to complete it with some tests as you want and merge it. Sorry for the delay.

dinosaure commented 9 months ago

I added tests to check if we release all ressources even if we use Test.multiple. Thanks for your patch, I will release as soon as I can bechamel 🎉 .

edwintorok commented 9 months ago

Thanks for the tests, I should probably undraft this PR now.