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

limits/imap.go: Prevent integer overflow on 32 bit platforms #383

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

benthetechguy
Copy link
Contributor

Fixes #382
Since the (signed) int type is used, MaxUInt32 is too big for use on 32 bit platforms. Changing to MaxInt32 solves this problem.

limits/imap.go Outdated Show resolved Hide resolved
@benthetechguy benthetechguy changed the title Use MaxInt32 instead of MaxUInt32 for 32 bit arch support Use uint32 for limits to prevent integer overflow on 32 bit platforms Jul 11, 2023
limits/imap.go Outdated Show resolved Hide resolved
@benthetechguy
Copy link
Contributor Author

benthetechguy commented Jul 12, 2023 via email

@LBeernaertProton
Copy link
Contributor

LBeernaertProton commented Jul 12, 2023

Is there any reason we can't just use MaxInt32 for limits on all systems, like my original changes did?

The maximum range of values we can represent with MaxInt32 is [0, 2147483647]

The maximum range of values we can represent with MaxUint32 is [0, 4294967295]

The current setup works, since int on 64bit system is equal to int64, which can safely express all the range of operations we need to perform to make the calculations.

While your original change compiles, it has some problems:

  • 64bit systems are now handicapped (max range of values reduced by 2)
  • Once we hit MaxInt32 values, the checks will always fail on 32bit system without due to signed integer overflow

@benthetechguy
Copy link
Contributor Author

benthetechguy commented Jul 12, 2023 via email

@LBeernaertProton
Copy link
Contributor

So you're okay with 32 bit platforms having half the max range as long as 64 bit ones don't also?

Yes, 64bit should have full range. I see this as compromise for now.

There is much more work required to ensure that one can represent the max range of uint32 correctly across the entire code base in 32bit mode. But at least it would not block people from building under 32bits.

@benthetechguy benthetechguy changed the title Use uint32 for limits to prevent integer overflow on 32 bit platforms limits/imap.go: Prevent integer overflow on 32 bit platforms Jul 13, 2023
limits/imap.go Outdated Show resolved Hide resolved
@LBeernaertProton
Copy link
Contributor

Looks good now. Could you amend the commit to fix: Prevent IMAP Limits integer overflow on 32 bit platforms

@benthetechguy
Copy link
Contributor Author

I don't know if you were notified, but I did it three days ago.

@LBeernaertProton LBeernaertProton enabled auto-merge (rebase) July 17, 2023 06:22
@LBeernaertProton LBeernaertProton merged commit f4bd9fd into ProtonMail:dev Jul 17, 2023
4 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.

32 bit support
2 participants