refactor: Misc int sign change fixes

[maflcko]

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.

[DrahtBot]

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]

My logs:

qt/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)
    #0 0x55d97136bb7f in WalletView::processNewTransaction(QModelIndex const&, int, int) src/qt/walletview.cpp:137:21

SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation-or-sign-change qt/notificator.cpp:117:40 in 
qt/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)
    #0 0x55d971221ca2 in FreedesktopImage::FreedesktopImage(QImage const&) src/qt/notificator.cpp:118:40
    #1 0x55d97122200e in FreedesktopImage::toVariant(QImage const&) src/qt/notificator.cpp:140:22
    #2 0x55d97122251a in Notificator::notifyDBus(Notificator::Class, QString const&, QString const&, QIcon const&, int) src/qt/notificator.cpp:190:26
    #3 0x55d9711b47f3 in BitcoinGUI::message(QString const&, QString, unsigned int, bool*, QString const&) src/qt/bitcoingui.cpp:1267:22
    #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

wallet/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)
    #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
    #1 0x55dcc9439953 in WalletModel::prepareTransaction(WalletModelTransaction&, wallet::CCoinControl const&) src/qt/walletmodel.cpp:211:37
    #2 0x55dcc9624ce5 in SendCoinsDialog::PrepareSendText(QString&, QString&, QString&) src/qt/sendcoinsdialog.cpp:287:28
    #3 0x55dcc960c98b in SendCoinsDialog::sendButtonClicked(bool) src/qt/sendcoinsdialog.cpp:482:10
    #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]

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

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

[hebasto]

Concept ACK.

[pablomartin4btc]

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.

[maflcko]

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

[pablomartin4btc]

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};

[pablomartin4btc]

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

Suggested change

	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

[maflcko]

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

[maflcko]

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};

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]

rfm, or is anything left to be done here?

[hebasto]

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