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

Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations #14224

Conversation

Projects
None yet
6 participants
@practicalswift
Copy link
Member

commented Sep 15, 2018

  • Add -fsanitize=integer Travis job
  • Document unsigned integer overflows (wraparounds) using annotation ALLOW_WRAPAROUND (intentional) or WARNING_UNINTENTIONAL_WRAPAROUND (unintentional).

@practicalswift practicalswift changed the title Annotate unsigned integer overflows. Add Travis job with -fsanitize=integer. Annotate unsigned integer overflows. Add -fsanitize=integer Travis job. Sep 15, 2018

@ken2812221

This comment has been minimized.

Copy link
Member

commented Sep 15, 2018

Concept ACK.

Show resolved Hide resolved .travis.yml Outdated
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14651 (Refactor: Fix compiler warning in prevector.h by ldm5180)
  • #14605 (Return of the Banman by dongcarl)
  • #14194 (Annotate unused parameters with [[maybe_unused]] by practicalswift)
  • #13949 (Introduce MempoolObserver interface to break "policy/fees -> txmempool -> policy/fees" circular dependency by Empact)
  • #13937 (Track best-possible-headers (TheBlueMatt) by Sjors)
  • #13864 (validation: Document where we are intentionally ignoring bool return values from validation related functions by practicalswift)
  • #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
  • #13804 (WIP: Transaction Pool Layer by MarcoFalke)
  • #13062 (Make script interpreter independent from storage type CScript by sipa)
  • #11535 (Avoid unintentional unsigned integer wraparounds by practicalswift)
  • #10729 (Wrap EvalScript in a ScriptExecution class by luke-jr)
  • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@arvidn

This comment has been minimized.

Copy link

commented Sep 16, 2018

there are a few #include<> directives that changed order, was that deliberate?
I take it the idea is to make new code that relies on unsigned wrap-around semantics to be annotated, and current annotations to be removed as code is fixed. Is that right?
would it make sense to use a different name for the macro? something like: ALLOW_WRAPAROUND.

nit: technically unsigned integers don't overflow, all operators on unsigned have modulo-2 arithmetic, so all results will always fit in the resulting variable.

@practicalswift practicalswift force-pushed the practicalswift:no_sanitize-unsigned-integer-overflow branch Sep 16, 2018

@practicalswift practicalswift changed the title Annotate unsigned integer overflows (wraparounds). Add -fsanitize=integer Travis job. Add -fsanitize=integer Travis job. Annotate unsigned integer overflows (wraparounds). Sep 16, 2018

@practicalswift practicalswift changed the title Add -fsanitize=integer Travis job. Annotate unsigned integer overflows (wraparounds). Add -fsanitize=integer Travis job. Document unsigned integer overflows (wraparounds) using annotation ALLOW_WRAPAROUND. Sep 16, 2018

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

Please don't just blindly permit existing cases that might actually be bugs. If we must do this with bypasses, the annotation macro should be different for cases where the modular arithmetic is intentional (e.g. hash functions) and where we're just silencing the warning in order to get something merged.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2018

@arvidn ALLOW_WRAPAROUND is a much better name. Updated!

Yes, the include sorting was deliberate.

Yes, the idea is to make it so that new code that relies on unsigned wrap-around semantics is annotated. I've now added the following to the developer notes to clarify:

Do not rely on unsigned wrap-around semantics unless there are good reasons for doing so (e.g. hashing). Functions that rely on on unsigned wrap-around semantics should be annotated using ALLOW_WRAPAROUND.

  • Rationale: Code relying on unsigned wrap-around semantics is legal and well-defined but error-prone.

Looks good?

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

I don't agree that it is error prone. I'm aware of no study which shows as much, and it's not my experience that intentional use of modular arithmetic results in bugs.

The comment should state that intentional use of it needs to be annotated because unintended wraps are often bugs and we use instrumentation to look for those bugs.

But so long as the patch just papers over behaviour that is probably buggy-- or only not buggy by chance--, I think my position is NAK.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2018

@gmaxwell The bulk of the existing ones were handled in #11535 which was submitted almost a year ago :-)

My main goal with this PR is to make sure we're not introducing new unintentional integer wrap arounds. Until #11535 is merged I'm afraid we need ALLOW_WRAPAROUND to make -fsanitize=integer pass, no?

Please advice on how to proceed :-)

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2018

@gmaxwell Yes, intentional use of wraparound semantics (e.g. hashing code) is obviously not problematic. I'll update the developer note to make that crystal clear.

@practicalswift

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2018

@gmaxwell

I don't agree that it is error prone. I'm aware of no study which shows as much

From the paper "Understanding Integer Overflow in C/C++" co-authored by Regehr:

IOC’s instrumentation is designed to be semantically transparent for programs that conform to the C or C++ language standards, except in the case where a user requests additional checking for conforming but error-prone operations, e.g., wraparound with unsigned integer types.

.-)

@practicalswift practicalswift force-pushed the practicalswift:no_sanitize-unsigned-integer-overflow branch Sep 16, 2018

@practicalswift practicalswift force-pushed the practicalswift:no_sanitize-unsigned-integer-overflow branch Sep 22, 2018

@DrahtBot DrahtBot removed the Needs rebase label Sep 22, 2018

@practicalswift practicalswift force-pushed the practicalswift:no_sanitize-unsigned-integer-overflow branch Sep 30, 2018

@practicalswift practicalswift force-pushed the practicalswift:no_sanitize-unsigned-integer-overflow branch Nov 5, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 5, 2018

@practicalswift practicalswift force-pushed the practicalswift:no_sanitize-unsigned-integer-overflow branch 2 times, most recently Nov 5, 2018

@practicalswift practicalswift force-pushed the practicalswift:no_sanitize-unsigned-integer-overflow branch Nov 6, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 6, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Nov 6, 2018

@bitcoin bitcoin deleted a comment from practicalswift Nov 6, 2018

@bitcoin bitcoin deleted a comment from practicalswift Nov 6, 2018

@bitcoin bitcoin deleted a comment from practicalswift Nov 6, 2018

@bitcoin bitcoin deleted a comment from practicalswift Nov 6, 2018

practicalswift added some commits Sep 14, 2018

Document unsigned integer wraparounds with ALLOW_WRAPAROUND (intentio…
…nal) or WARNING_UNINTENTIONAL_WRAPAROUND (unintentional)

@practicalswift practicalswift force-pushed the practicalswift:no_sanitize-unsigned-integer-overflow branch to 0373038 Nov 12, 2018

@DrahtBot DrahtBot removed the Needs rebase label Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.