refactor: Misc int sign change fixes #806
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
My logs: |
|
There is still one more, which I don't know how it happened and may or may not be a real issue: |
|
Concept ACK. |
There was a problem hiding this comment.
tACK 0541642
I've reproduced the 3 errors described within this PR.
Tested on Ubuntu 22.04, this PR fixes 2 of the 3 errors.
2nd. commit 321f105 doesn't fix error UndefinedBehaviorSanitizer: implicit-signed-integer-truncation-or-sign-change on qt/notificator.cpp which is only shown when using --with-sanitizers=undefined,integer, using --with-sanitizers=integer doesn't produce the error.
|
@pablomartin4btc Thank you for spending the time to reproduce and review each commit! |
There was a problem hiding this comment.
I've been playing a bit with the code and found out that the problem was a mix of the fix you provided in the 2nd commit, plus the initialisation of hasAlpha in the constructor of FreedesktopImage. It seems setting it in the private: section of the class definition works and the error is not raised 🙄.
private:
int width, height, stride;
bool hasAlpha{true};
| image[ptr * BYTES_PER_PIXEL + 0] = char(data[ptr] >> 16); // R | ||
| image[ptr * BYTES_PER_PIXEL + 1] = char(data[ptr] >> 8); // G | ||
| image[ptr * BYTES_PER_PIXEL + 2] = char(data[ptr]); // B | ||
| image[ptr * BYTES_PER_PIXEL + 3] = char(data[ptr] >> 24); // A |
There was a problem hiding this comment.
nit: since we are touching this, perhaps we could use static_cast<char> instead of the C-style?
| image[ptr * BYTES_PER_PIXEL + 0] = char(data[ptr] >> 16); // R | |
| image[ptr * BYTES_PER_PIXEL + 1] = char(data[ptr] >> 8); // G | |
| image[ptr * BYTES_PER_PIXEL + 2] = char(data[ptr]); // B | |
| image[ptr * BYTES_PER_PIXEL + 3] = char(data[ptr] >> 24); // A | |
| image[ptr * BYTES_PER_PIXEL + 0] = static_cast<char>(data[ptr] >> 16); // R | |
| image[ptr * BYTES_PER_PIXEL + 1] = static_cast<char>(data[ptr] >> 8); // G | |
| image[ptr * BYTES_PER_PIXEL + 2] = static_cast<char>(data[ptr]); // B | |
| image[ptr * BYTES_PER_PIXEL + 3] = static_cast<char>(data[ptr] >> 24); // A |
There was a problem hiding this comment.
I don't think there is a difference for integral values, other than one being more to type and read
I see. So this is an actual uninitialized read (UB)? I think this should be fixed separate from a refactor that only documents that the code is correct and the integer sanitizer can be silent about them. |
|
rfm, or is anything left to be done here? |
This is allowed by the language. However, the
integersanitizer complains about it. Thus, fix it, so that theintegersanitizer can be used in the future to catch unintended sign changes.Fixes #805.