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

Integer Overflow or Wraparound #1675 #1683

Closed
wants to merge 3 commits into from
Closed

Integer Overflow or Wraparound #1675 #1683

wants to merge 3 commits into from

Conversation

ovidiu-benea
Copy link

Added check to determine if overflow/wrapping will occur

Added check to determine if overflow/wrapping will occur
@mention-bot
Copy link

@ovidiu-benea, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @linusnielsen to be potential reviewers.

@coveralls
Copy link

Coverage Status

Coverage decreased (-75.3%) to 0.0% when pulling 9b4666a on ovidiu-benea:master into 798ad5d on curl:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-75.3%) to 0.0% when pulling d976580 on ovidiu-benea:master into 798ad5d on curl:master.

Fix checksrc.pl errors
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 75.28% when pulling 0c12574 on ovidiu-benea:master into 798ad5d on curl:master.

@bagder
Copy link
Member

bagder commented Jul 21, 2017

I'm concerned about a few things with this patch:

  1. I fear SIZE_MAX is not going to be universally available unless you make sure the dedicated header for it is included, and I suspect that it might be troublesome on legacy unixes. Just a suspicion. This can probably be addressed by an #ifdef that skips the test if the symbol doesn't exist.
  2. Modern architectures have size_t as 64 bits and sockets are ints, 32 bits. An application thus can only keep a theoretical limit of 4 billion sockets (probably just 2 billion if we stick to positive integers). That's then not enough for this wrap-around to occur. So this check is not likely to ever trigger within a foreseeable future. What's the purpose of it again? A system with 32bit size_t still needs 2^29 allocated sockets, which is 500 million. Not likely to happen either.
  3. I would like to verify this check with a unit test, which according to 2 has to be a broken application since it can't actually allocate that many sockets.

I propose we instead solve this issue with a comment in the source code!

@ovidiu-benea
Copy link
Author

Hi,
Thanks for looking into this. I understand that this is not likely to happen. If an #ifdef that skips the test if SIZE_MAX doesn't exist is not a suitable fix, then we can add a comment.

@bagder bagder closed this in 02c7a2c Jul 26, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants