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 division of numerator 0 #310

Merged
merged 1 commit into from
Aug 6, 2024
Merged

Fix division of numerator 0 #310

merged 1 commit into from
Aug 6, 2024

Conversation

chfast
Copy link
Owner

@chfast chfast commented Aug 5, 2024

In case the division numerator is 0 its computed number of words m is also 0. Then avoid accessing un[m-1].

This is not a critical bug because in the worst case we increased the m by 1 but results were computed correctly.

@chfast chfast added the bug label Aug 5, 2024
@chfast chfast requested a review from gumb0 August 5, 2024 12:15
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (150f2de) to head (a54970b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #310   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          10       10           
  Lines        1890     1890           
=======================================
  Hits         1889     1889           
  Misses          1        1           
Flag Coverage Δ
32bit 99.94% <100.00%> (ø)
gcc 99.41% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
include/intx/intx.hpp 99.86% <100.00%> (ø)

@gumb0
Copy link
Collaborator

gumb0 commented Aug 6, 2024

This is not a critical bug because in the worst case we increased the m by 1 but results were computed correctly.

It could segfault theoretically on out of buffer access?

@chfast
Copy link
Owner Author

chfast commented Aug 6, 2024

This is not a critical bug because in the worst case we increased the m by 1 but results were computed correctly.

It could segfault theoretically on out of buffer access?

No. The un is followed by vn in the normalized_div_args struct. So we were accessing the lowest word of the vn. So the comparison was garbage but not invalid memory access.

@chfast
Copy link
Owner Author

chfast commented Aug 6, 2024

In other words, executing tests in constexpr works as pretty good undefined behavior sanitizer :)

@chfast chfast force-pushed the refactor_div_loops branch from 3886b49 to 5510a01 Compare August 6, 2024 09:21
Base automatically changed from refactor_div_loops to master August 6, 2024 09:43
In case the division numerator is 0 its computed number of words `m`
is also 0. Then avoid accessing `un[m-1]`.

This is not a critical bug because in the worst case we increased
the `m` by 1 but results were computed correctly.
Copy link

sonarqubecloud bot commented Aug 6, 2024

@chfast chfast merged commit 6cf12a4 into master Aug 6, 2024
19 checks passed
@chfast chfast deleted the fix_div0 branch August 6, 2024 10:12
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 this pull request may close these issues.

2 participants