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

Fix freeing non-allocated memory when copy shape_type #444

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

KHLee529
Copy link
Contributor

@KHLee529 KHLee529 commented Dec 22, 2024

This PR fixed the bug described in Issue #437.

Bug Description

Memory crash happened when constructing a SimpleArray from np.ndarray. This was due to freeing the memory of non-allocated memory in the copy assignment operator of shape_type aka. small_vector<size_t>.

This bug only happens when the size of the shape of the source np.ndarray is larger then 3

Bug Backtrace

In File <cpp/modmesh/buffer/small_vector.hpp> line 146: delete[] m_head;
In File <cpp/modmesh/buffer/SimpleArray.hpp> line 351: m_shape = shape;
In File <cpp/modmesh/buffer/SimpleArray.hpp> line 317: SimpleArray(shape, stride, buffer)
In File <cpp/modmesh/buffer/pymod/wrap_SimpleArray.hpp> line 94: return wrapped_type(shape, stride, buffer, is_c_contiguous, is_f_contiguous);

Solution

The delete statement where the error happens are intended to remove the allocated buffer when the source vector has larger capacity than the target vector. However when deleting the array, it wasn't check whether the buffer was allocated or not. Therefore, this PR adds this check. Also I went through all the constructors and assignment operators of small_vector and confirmed that this issue doesn't happen in other place.

@yungyuc yungyuc added bug Something isn't working array Multi-dimensional array implementation labels Dec 22, 2024
@ThreeMonth03
Copy link
Collaborator

@yungyuc Should we write the unittest to check the functionality of the small vector?

@yungyuc
Copy link
Member

yungyuc commented Dec 22, 2024

Thanks a lot for catching this! Mistake from me: 2a1c689#diff-d2ce5a8e02d44440a1f3a13051001c758fb6fdbd1bb01f2754a024b3812a657fR112

@ThreeMonth03 It is hard to make dedicated tests for errors like this, but the testing code in issue #437 should be added to unit tests. Testing high-level behaviors is sufficient for preventing regression.

@yungyuc yungyuc merged commit 0060937 into solvcon:master Dec 22, 2024
12 checks passed
@yungyuc
Copy link
Member

yungyuc commented Dec 22, 2024

@KHLee529 @ThreeMonth03 could (one of) you please add a unit test in a followup PR?

@yungyuc
Copy link
Member

yungyuc commented Dec 22, 2024

@KHLee529 Next time please use a unique branch to create PR. Using master or any existing branch for PR makes it hard to pull your change without disrupting existing code development.

(Avoid what you did in this PR:)

image

@KHLee529
Copy link
Contributor Author

@yungyuc Got it. I'll make a new branch next time.
I thought both master will somehow be different when pull to local but I realized that is not the way git does.
Thank you for the advice!

@yungyuc
Copy link
Member

yungyuc commented Dec 23, 2024

No problem. Don't forget to add the unit test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
array Multi-dimensional array implementation bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Incorrect construction of SimpleArray from a high-dimension ndarray
3 participants