This commit makes the behavior defined (unsigned integer overflow as if:
it->second.nscore = static_cast<int>(static_cast<unsigned> (it->second.nscore) + 1U);)
See: Issue #24049; Discussion on stackoverflow.
Fix #24049: signed integer overflow in `SeenLocal` #26399
pull ViralTaco wants to merge 4 commits into bitcoin:master from ViralTaco:patch-1 changing 1 files +1 −1-
ViralTaco commented at 2:09 AM on October 27, 2022: none
-
474504fa4a
Fix #24049: signed integer overflow in `SeenLocal`
This commit makes the code valid.
- ViralTaco marked this as ready for review on Oct 27, 2022
-
hebasto commented at 8:19 AM on October 27, 2022: member
Did you see a discussion in #24090? Especially, #24090 (comment) and #24090 (comment)?
-
DrahtBot commented at 2:09 PM on October 27, 2022: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
See the guideline for information on the review process. A summary of reviews will appear here.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)
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.
-
luke-jr commented at 5:32 PM on October 27, 2022: member
Doesn't appear to actually fix the issue though?
- DrahtBot cross-referenced this on Oct 27, 2022 from issue sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild
-
Merge branch 'bitcoin:master' into patch-1 1910df5463
- maflcko added the label Needs rebase on Oct 31, 2022
- DrahtBot removed the label Needs rebase on Oct 31, 2022
-
Merge branch 'bitcoin:master' into patch-1 58d9a4fd58
-
Merge branch 'bitcoin:master' into patch-1 a1f1af1cde
-
maflcko commented at 2:33 PM on November 18, 2022: member
Looking at the previous comments, this doesn't seem to be a fix. Let's continue discussion in the issue for now.
- maflcko closed this on Nov 18, 2022
-
ViralTaco commented at 7:20 PM on November 20, 2022: none
Looking at the previous comments, this doesn't seem to be a fix. Let's continue discussion in the issue for now.
It doesn't "seem[sic]" to fix the issue?
What issue? The code before this patch has undefined behavior. It can do anything, but it doesn't have to. It could do nothing.The first sentence in your comment only makes sense in a vacuum. The standard is a thing. I agree with the second part. Let's.
-
maflcko commented at 7:42 PM on November 20, 2022: member
The burden to prepare, explain and motivate a change for reviewers is on the author. You created a one-line patch in 4 commits, ignored previous discussions, ignored two review comments, with no apparent progress in about a month. I closed this for now, because it can't be merged according to the guidelines. At a minimum for this simple one line change, you'll have to create at most one commit, with an explanation, and you'll have to reply to review comments. All of this is explained in https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md
- ViralTaco cross-referenced this on Nov 24, 2022 from issue net: signed-integer-overflow in LocalServiceInfo by Crypt-iQ
- glozow cross-referenced this on Feb 1, 2023 from issue mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool by glozow
- bitcoin locked this on Nov 20, 2023
