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

BUG: vnl_matrix move-assignment segfault #701

Merged
merged 2 commits into from
Oct 9, 2019

Conversation

drewgilliam
Copy link
Contributor

In the move-assignment operator= for vnl_matrix (see PR #694) when both left-hand side and right-hand side objects manage their own memory, the right-hand side object should finally be invalidated with null data, zero size, and m_LetArrayManageMemory set to a default value of true (not false). The system segfaults should a user attempt to std::swap two matrices.

Set m_LetArrayManageMemory to true in move-assignment operator= for both vnl_matrix and vnl_vector. Add more swap tests, including native swap and std::swap for both vnl_matrix and vnl_vector.

More Details

During move-assignment, vnl_matrix wrongly reset m_LetArrayManageMemory to false. This is an issue should a user attempt to std:swap two matrices (while users should prefer the native vnl_matrix::swap function, std::swap should certainly not segfault).

The C++11 std::swap method is implemented as follows

template<typename T> void swap(T& t1, T& t2) {
    T temp = std::move(t1); // line (1)
    t1 = std::move(t2);     // line (2)
    t2 = std::move(temp);   // line (3)
}

When using std::swap on a vnl_matrix, matrix t1 would be cleared after line 1 but with m_LetArrayManageMemory = false. Line two would attempt to follow a code path for externally managed memory and the system would segfault.

PR Checklist

🚫 Makes breaking changes to the vxl/core/* API that requires semantic versioning increase
🚫 Makes design changes to existing vxl/core* API that requires semantic versioning increase
🚫 Makes changes to the contributed directory API DOES NOT require semantic versioning increase
✔️ Adds tests and baseline comparison (quantitative).
🚫 Adds Documentation.

@drewgilliam
Copy link
Contributor Author

@hjmjohnson

@hjmjohnson hjmjohnson self-assigned this Oct 9, 2019
@hjmjohnson hjmjohnson merged commit e9ba886 into vxl:master Oct 9, 2019
@drewgilliam drewgilliam deleted the std_swap branch October 17, 2019 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants