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
base: master
from

Conversation

Projects
None yet
4 participants
@ovidiu-benea

ovidiu-benea commented Jul 18, 2017

Added check to determine if overflow/wrapping will occur

Integer Overflow or Wraparound #1675
Added check to determine if overflow/wrapping will occur
@mention-bot

This comment has been minimized.

mention-bot commented Jul 18, 2017

@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

This comment has been minimized.

coveralls commented Jul 18, 2017

Coverage Status

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

Integer Overflow or Wraparound #1675
Fix Travis CI check
@coveralls

This comment has been minimized.

coveralls commented Jul 18, 2017

Coverage Status

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

Integer Overflow or Wraparound #1675
Fix checksrc.pl errors
@coveralls

This comment has been minimized.

coveralls commented Jul 18, 2017

Coverage Status

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

@bagder

This comment has been minimized.

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

This comment has been minimized.

ovidiu-benea commented Jul 25, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment