Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix double free in Test.uniq #47

Merged
merged 3 commits into from
Oct 31, 2023
Merged

Fix double free in Test.uniq #47

merged 3 commits into from
Oct 31, 2023

Conversation

edwintorok
Copy link
Contributor

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
Copy link
Contributor Author

(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
Copy link
Member

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

@dinosaure
Copy link
Member

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
Copy link
Contributor Author

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

@edwintorok edwintorok marked this pull request as ready for review October 31, 2023 14:00
@dinosaure dinosaure merged commit 9447615 into mirage:main Oct 31, 2023
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Oct 31, 2023
CHANGES:

- Fix unsafe access causing SIGSEGV (@edwintorok, mirage/bechamel#43)
- Fix compulation of the confidence indicator (@edwintorok, @lindig, @dinosaure, mirage/bechamel#45)
- Upgrade monotonic clock detection (@dra27, mirage/bechamel#44)
- Fix double-free when we use resources (@edwintorok, @dinosaure, mirage/bechamel#47)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

- Fix unsafe access causing SIGSEGV (@edwintorok, mirage/bechamel#43)
- Fix compulation of the confidence indicator (@edwintorok, @lindig, @dinosaure, mirage/bechamel#45)
- Upgrade monotonic clock detection (@dra27, mirage/bechamel#44)
- Fix double-free when we use resources (@edwintorok, @dinosaure, mirage/bechamel#47)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants