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

int idx --> size_t idx to prevent integer overflow #720

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

tuan-karma
Copy link
Contributor

@tuan-karma tuan-karma commented Oct 7, 2023

for(int idx = number * number; idx <= limit; idx +=number)
	composite[idx] = true;

This code can be integer overflow if the limit > sqrt(maxInt).
--> Then it leads to wrongly admit that idx <= limit.
--> Then it leads to an out of bounds access at composite[idx] = true then it leads to Segmentation Fault (for example, when you set limit = 47000).

```
for(int idx = number * number; idx <= limit; idx +=number)
	composite[idx] = true;
```
This code can be integer overflow if the limit > sqrt(maxInt). Then it leads to wrongly admit that idx <= limit. Then it leads to an out of bounds access at `composite[idx] = true` then it leads to "Segmentation Fault` (for example, when you set limit = 47000).
This is a minor correction of patch-1
@siebenschlaefer
Copy link
Contributor

Yes, there's a potential integer overflow if the limit is too big. Good catch!

But the suggested change doesn't fix that, the underlying type of the operation number * number is still int, you will still get an integer overflow if the limit is too big, before the conversion to std::size_t.
Also, I would recommend not to mix signed and unsigned types in the same expression (see C++CG).

How about defining number and idx as long long or std::int64_t? That should solve the problem, at least on platforms where an int has fewer than than 64 bits. What do you think?

@vaeng
Copy link
Contributor

vaeng commented Feb 12, 2024

4 months have passed without a reaction from the original poster. I am thus merging it with SIebenschläfers suggestions. I hope that is okay. Tuan should still get reputation points on Exercism.

@vaeng vaeng merged commit 465b10d into exercism:main Feb 12, 2024
7 checks passed
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