Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Bigarray mmap leak #8568

Merged
merged 4 commits into from Apr 2, 2019

Conversation

Projects
None yet
3 participants
@damiendoligez
Copy link
Member

commented Apr 1, 2019

When a bigarray is allocated with Unix.mmap_file then passed to slice, change_layout, reshape or sub, a new bigarray is allocated that shares the data. But this allocation is done in bigarray.c with the normal allocation function, using the same flags as the original array, in particular the CAML_BA_MAPPED_FILE flag. The normal allocation function sets the finalization function to caml_ba_finalize, which doesn't expect a mapped bigarray, and in this case does nothing (in normal mode) or asserts false (in debug mode).

This results in a leak of memory (the proxy block) and address space (the mmapped file) and possibly other system resources.

The solution is to copy the custom_ops pointer from the original array, ensuring that the proper finalization function is called.

I tried to come up with a test but the leak is quite slow on 64-bit machines so I couldn't get a clean crash.

@damiendoligez damiendoligez force-pushed the damiendoligez:bigarray-mmap-leak branch from 21eb196 to 0d9e56c Apr 1, 2019

@damiendoligez damiendoligez requested a review from xavierleroy Apr 1, 2019

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

I've put the Changes entry in the 4.08 section because I think this should be cherry-picked to 4.08.

@xavierleroy
Copy link
Contributor

left a comment

Looks good to me.

@xavierleroy xavierleroy added the bug label Apr 1, 2019

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

@diml : would you be available for a second review? Thanks.

@diml

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Sure. I read the patch and it looks good to me. BTW, it seems to me that in caml_ba_finalize we could systematically fail when seeing a mapped bigarray, that would help detect such errors earlier. I'm guessing that assertions are disabled in release mode for performance reasons, but in this case the CAMLassert(0) is already in the error branch.

@xavierleroy xavierleroy force-pushed the damiendoligez:bigarray-mmap-leak branch from 23fdc7f to 1131919 Apr 2, 2019

@xavierleroy xavierleroy merged commit aafcd12 into ocaml:trunk Apr 2, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Merged in trunk and cherry-picked to 4.08.

The 4.08 commits are: 1799a7e 834d77c 879efc8 9841cec.

@damiendoligez damiendoligez deleted the damiendoligez:bigarray-mmap-leak branch Apr 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.