refactor: Misc int sign change fixes

maflcko commented at 6:45 pm on March 21, 2024: contributor

This is allowed by the language. However, the integer sanitizer complains about it. Thus, fix it, so that the integer sanitizer can be used in the future to catch unintended sign changes.

Fixes #805.

refactor: Avoid implicit-integer-sign-change in createTransaction 6d8eecd33a

refactor: Avoid implicit-signed-integer-truncation-or-sign-change in FreedesktopImage 321f105d08

refactor: Avoid implicit-integer-sign-change in processNewTransaction 05416422d3

DrahtBot commented at 6:45 pm on March 21, 2024: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#807 (refactor: interfaces, make ‘createTransaction’ less error-prone by furszy)

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.

maflcko commented at 6:46 pm on March 21, 2024: contributor

My logs:

0qt/walletview.cpp:137:21: runtime error: implicit conversion from type 'qulonglong' (aka 'unsigned long long') of value 18446744068709551616 (64-bit, unsigned) to type 'qint64' (aka 'long long') changed the value to -5000000000 (64-bit, signed)
1    [#0](/bitcoin-core-gui/0/) 0x55d97136bb7f in WalletView::processNewTransaction(QModelIndex const&, int, int) src/qt/walletview.cpp:137:21

0SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation-or-sign-change qt/notificator.cpp:117:40 in 
1qt/notificator.cpp:118:40: runtime error: implicit conversion from type 'uint32_t' (aka 'unsigned int') of value 255 (32-bit, unsigned) to type 'char' changed the value to -1 (8-bit, signed)
2    [#0](/bitcoin-core-gui/0/) 0x55d971221ca2 in FreedesktopImage::FreedesktopImage(QImage const&) src/qt/notificator.cpp:118:40
3    [#1](/bitcoin-core-gui/1/) 0x55d97122200e in FreedesktopImage::toVariant(QImage const&) src/qt/notificator.cpp:140:22
4    [#2](/bitcoin-core-gui/2/) 0x55d97122251a in Notificator::notifyDBus(Notificator::Class, QString const&, QString const&, QIcon const&, int) src/qt/notificator.cpp:190:26
5    [#3](/bitcoin-core-gui/3/) 0x55d9711b47f3 in BitcoinGUI::message(QString const&, QString, unsigned int, bool*, QString const&) src/qt/bitcoingui.cpp:1267:22
6    [#4](/bitcoin-core-gui/4/) 0x55d9711b05af in BitcoinGUI::incomingTransaction(QString const&, BitcoinUnits::Unit, long const&, QString const&, QString const&, QString const&, QString const&) src/qt/bitcoingui.cpp:1348:5

0wallet/interfaces.cpp:289:57: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
1    [#0](/bitcoin-core-gui/0/) 0x55dcca43aaea in wallet::(anonymous namespace)::WalletImpl::createTransaction(std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, wallet::CCoinControl const&, bool, int&, long&) src/wallet/interfaces.cpp:289:57
2    [#1](/bitcoin-core-gui/1/) 0x55dcc9439953 in WalletModel::prepareTransaction(WalletModelTransaction&, wallet::CCoinControl const&) src/qt/walletmodel.cpp:211:37
3    [#2](/bitcoin-core-gui/2/) 0x55dcc9624ce5 in SendCoinsDialog::PrepareSendText(QString&, QString&, QString&) src/qt/sendcoinsdialog.cpp:287:28
4    [#3](/bitcoin-core-gui/3/) 0x55dcc960c98b in SendCoinsDialog::sendButtonClicked(bool) src/qt/sendcoinsdialog.cpp:482:10
5    [#4](/bitcoin-core-gui/4/) 0x55dcc9665fdc in auto auto GUIUtil::ExceptionSafeConnect<QPushButton*, void (QAbstractButton::*)(bool), SendCoinsDialog*, void (SendCoinsDialog::*)(bool)>(QPushButton*, void (QAbstractButton::*)(bool), SendCoinsDialog*, void (SendCoinsDialog::*)(bool), Qt::ConnectionType)::'lambda'(auto&&...)::operator()<bool&>(auto&&...) const src/./qt/guiutil.h:400:21

maflcko commented at 6:49 pm on March 21, 2024: contributor

There is still one more, which I don’t know how it happened and may or may not be a real issue:

0SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation-or-sign-change qt/notificator.cpp:118:40 in 
1qt/notificator.cpp:125:47: runtime error: load of value 95, which is not a valid value for type 'bool'
2    [#0](/bitcoin-core-gui/0/) 0x55d971221f27 in operator<<(QDBusArgument&, FreedesktopImage const&) src/qt/notificator.cpp:125:47

hebasto commented at 9:19 am on March 22, 2024: member

Concept ACK.

pablomartin4btc commented at 0:30 am on March 28, 2024: contributor

tACK 05416422d354b29d59558ce227e076028338b442

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 321f105d08ddf958881908ea57ad263ffdccd225 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.

DrahtBot requested review from hebasto on Mar 28, 2024

maflcko commented at 6:13 am on March 28, 2024: contributor

@pablomartin4btc Thank you for spending the time to reproduce and review each commit!

in src/qt/notificator.cpp:118 in 05416422d3

117-        image[ptr*BYTES_PER_PIXEL+2] = data[ptr];       // B
118-        image[ptr*BYTES_PER_PIXEL+3] = data[ptr] >> 24; // A
119+        image[ptr * BYTES_PER_PIXEL + 0] = char(data[ptr] >> 16); // R
120+        image[ptr * BYTES_PER_PIXEL + 1] = char(data[ptr] >> 8);  // G
121+        image[ptr * BYTES_PER_PIXEL + 2] = char(data[ptr]);       // B
122+        image[ptr * BYTES_PER_PIXEL + 3] = char(data[ptr] >> 24); // A

pablomartin4btc commented at 3:47 pm on March 28, 2024:

nit: since we are touching this, perhaps we could use static_cast<char> instead of the C-style?

0        image[ptr * BYTES_PER_PIXEL + 0] = static_cast<char>(data[ptr] >> 16); // R
1        image[ptr * BYTES_PER_PIXEL + 1] = static_cast<char>(data[ptr] >> 8);  // G
2        image[ptr * BYTES_PER_PIXEL + 2] = static_cast<char>(data[ptr]);       // B
3        image[ptr * BYTES_PER_PIXEL + 3] = static_cast<char>(data[ptr] >> 24); // A

maflcko commented at 5:27 pm on March 28, 2024:

I don’t think there is a difference for integral values, other than one being more to type and read

pablomartin4btc commented at 3:51 pm on March 28, 2024: contributor

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 🙄.

0private:
1    int width, height, stride;
2    bool hasAlpha{true};

maflcko commented at 5:31 pm on March 28, 2024: contributor

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 🙄.
0private:
1    int width, height, stride;
2    bool hasAlpha{true};

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.

maflcko commented at 7:26 am on April 18, 2024: contributor

rfm, or is anything left to be done here?

hebasto approved

hebasto commented at 8:34 am on April 18, 2024: member

ACK 05416422d354b29d59558ce227e076028338b442, I have reviewed the code and it looks OK.

hebasto merged this on Apr 18, 2024

hebasto closed this on Apr 18, 2024

refactor: Misc int sign change fixes #806

Reviews

Conflicts