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

Memory leak in generic_resize/reallocate_copy #1378

Open
nickmertin opened this issue Apr 7, 2024 · 0 comments · May be fixed by #1379
Open

Memory leak in generic_resize/reallocate_copy #1378

nickmertin opened this issue Apr 7, 2024 · 0 comments · May be fixed by #1379
Labels

Comments

@nickmertin
Copy link

I believe there is a memory leak arising from DefaultAllocator::reallocate_copy and its use in Matrix::resize_generic. Anecdotally, I observed memory leakage in the wild in some pre-production code and narrowed its source down to here.

The resulting matrix is created by this line of resize_generic:

let res = unsafe { DefaultAllocator::reallocate_copy(new_nrows, new_ncols, data.data) };

In the case where at least one of the dimensions of the input matrix to resize_generic is Dyn, it is backed (by default) by a VecStorage; this is the type of data.data in the above line. However, in the case where the output matrix is statically sized, reallocate_copy moves the data out of this buffer to the new one, and then forgets the buffer:

// Safety:
// - We don’t care about dropping elements because the caller is responsible for dropping things.
// - We forget `buf` so that we don’t drop the other elements.
std::mem::forget(buf);

I believe the intention here is to ensure that the destructor does not get called on the elements of the old matrix, as the ones being used are moved to the new buffer, and the others are explicitly dropped by resize_generic; the latter part is, I believe, what is referred to by "the caller is responsible for dropping things." However, the caller cannot be responsible for dropping the buffer itself, as it is consumed by reallocate_copy; that is the source of the issue here.

I will try to put together a PR shortly to fix this and ensure that the allocation gets cleaned up, while maintaining memory safety regarding not double-dropping the matrix elements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants