Fix unsigned integer overflow in LoadMempool #24227
Conversation
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK aaaae5349d1d5985081751c3a3b7386333d48cef, I reviewed the code, and agree it makes sense to merge
|
I've pushed a new version, which:
|
There was a problem hiding this comment.
Code Review ACK fadcd03
Tried running below code to confirm:
uint64_t i = 4;
while (--i)
{
cout<<i;
}
cout<<i;
uint64_t i = 4;
while (i--)
{
cout<<i;
}
cout<<i;
uint64_t i = 4;
while (i)
{
--i;
cout<<i;
}
cout<<i;
It is also according to coding style mentioned in docs: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Is the PR name really accurate, this can't overflow, maybe only underflow?
|
No idea, but the suppression is called |
I presume there are more |
|
Have you seen the pull request you commented on? #24196 (review) |
Ahh, thanks, I forgot they were related / about that PR |
fadcd03 Fix unsigned integer overflow in LoadMempool (MarcoFalke) Pull request description: It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file. This removes one of the two violations. This should be a refactor. ACKs for top commit: prayank23: Code Review ACK bitcoin@fadcd03 Tree-SHA512: 9fb2f3d49008a59cd45b7c17be0c88c04e61183197c11c8176865af5532c8d0c940db49a351dd0fc75e1d7fd8678c3b816d34cfca170dc6b9cf8f37fdf1c8cae
fadcd03 Fix unsigned integer overflow in LoadMempool (MarcoFalke) Pull request description: It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file. This removes one of the two violations. This should be a refactor. ACKs for top commit: prayank23: Code Review ACK bitcoin@fadcd03 Tree-SHA512: 9fb2f3d49008a59cd45b7c17be0c88c04e61183197c11c8176865af5532c8d0c940db49a351dd0fc75e1d7fd8678c3b816d34cfca170dc6b9cf8f37fdf1c8cae
fadcd03 Fix unsigned integer overflow in LoadMempool (MarcoFalke) Pull request description: It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file. This removes one of the two violations. This should be a refactor. ACKs for top commit: prayank23: Code Review ACK bitcoin@fadcd03 Tree-SHA512: 9fb2f3d49008a59cd45b7c17be0c88c04e61183197c11c8176865af5532c8d0c940db49a351dd0fc75e1d7fd8678c3b816d34cfca170dc6b9cf8f37fdf1c8cae
Summary: This should have been backported before removing the ubsan suppression in D12821 This is a backport of [[bitcoin/bitcoin#24227 | core#24227]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12914
3 participants
It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.
This removes one of the two violations.
This should be a refactor.