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

check integer overflow in Matrix::setSize #296

Merged
merged 4 commits into from
Aug 8, 2024
Merged

Conversation

dreamer2368
Copy link
Collaborator

So far, Matrix does not check if the new size is overflowed. This returns a memory error far downstream of application, which takes a long time to backtrace.

Now Matrix::setSize checks if the new_size is overflowed at its execution.

@dylan-copeland
Copy link
Collaborator

This check is a good improvement, but I think there is a case in the constructor at Matrix.cpp:133, where setSize is not called, and the check should be added there as well.

Copy link
Collaborator

@ckendrick ckendrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this check. This might be a good time to also add this to Vector::setSize as well. Vector has another case where this check should be added to the constructor at Vector.cpp:69.

@dreamer2368
Copy link
Collaborator Author

@dylan-copeland suggestion reflected.

@ckendrick I looked into Vector class, and I think it's actually not the case with the same issue. For Matrix, the size is determined by multiplication inside the class, while for Vector the size is determined simply by a passed input value. We can only check overflow of internal operations. There is no way we can check if the passed dim is overflowed or not: a negative overflown value will be caught by dim > 0; but a positive overflown value, we cannot tell whether that is proper or overflown.

Also for Vector it is easier for users to check if the passed dim is a wrong value, while for Matrix we need to check the internal d_alloc_size.

@dreamer2368 dreamer2368 requested a review from ckendrick August 5, 2024 18:21
@dreamer2368 dreamer2368 merged commit 98eda96 into master Aug 8, 2024
4 checks passed
@dylan-copeland dylan-copeland deleted the mat_size_overflow branch August 8, 2024 04:04
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.

3 participants