QApplication takes the command line arguments and parses them itself for some built in command line arguments that it has. We don't want any of those built in arguments, so instead give it dummy arguments.
To test, you can use the -reverse option. Without this patch, everything will appear right-to-left; things that were on the left side will be on the right and everything is right aligned.
After this patch, -reverse will now give a startup error since we do not support this argument.
QApplication takes the command line arguments and parses them itself
for some built in command line arguments that it has. We don't want
any of those built in arguments, so instead give it dummy arguments.
We don't want any of those built in arguments
Care to explain why?
Care to explain why?
They can do unexpected things and cause issues. Qt can also just add more arguments that do other stuff. We have a larger attack surface by allowing this.
@theuni might want to comment here, as he was asking about if the changes in #16386 would remove the ability to add qt-specific runtime args to bitcoin-qt ?.
168 | @@ -169,8 +169,11 @@ void BitcoinCore::shutdown() 169 | } 170 | } 171 | 172 | -BitcoinApplication::BitcoinApplication(interfaces::Node& node, int &argc, char **argv): 173 | - QApplication(argc, argv), 174 | +static int qt_argc = 1; 175 | +static const char* qt_argv = "bitcoin-qt";
const here, you don't need to const_cast below (which is dangerous, as now this will be put in a .rodata section and Qt might try to write it)
these can be inside the function scope
What function? Constructor?
My suggestion is to make the constructor private and add a static BitcoinApplication::create() where you could declare a temporary static argc and argv and the app.
* if you remove the `const` here, you don't need to `const_cast` below (which is dangerous, as now this will be put in a `.rodata` section and Qt might try to write it)
GCC complains if it is non const.
warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwrite-strings]
My suggestion is to make the constructor private and add a
static BitcoinApplication::create()where you could declare a ~temporary~ static argc and argv and the app.
That requires a copy constructor, and QApplication does not have one. Qt explicitly does not want things to be copied.
GCC complains if it is non const.
Could be:
static char qt_arg[] = "bitcoin-qt";
static char* qt_argv = qt_arg;
?
That requires a copy constructor, and QApplication does not have one. Qt explicitly does not want things to be copied.
Eh, whatever you do, please don't copy around QApplication objects, even if you could. It's a singleton. It's also definitely not necessary to solve this.
Concept ACK
Concept ACK
Concept ACK
Concept ACK: smaller attack surface is better.
Concept ACK. Codewise, I agree with @laanwj that casting const away and passing it to QApplication seems unfortunate.
How about this
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 5ce4f3c19..f38235b15 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -169,11 +169,8 @@ void BitcoinCore::shutdown()
}
}
-static int qt_argc = 1;
-static const char* qt_argv = "bitcoin-qt";
-
BitcoinApplication::BitcoinApplication(interfaces::Node& node):
- QApplication(qt_argc, const_cast<char **>(&qt_argv)),
+ QApplication(argc, const_cast<char **>(&argv)),
coreThread(nullptr),
m_node(node),
optionsModel(nullptr),
diff --git a/src/qt/bitcoin.h b/src/qt/bitcoin.h
index 3869193a3..d8b7f3b36 100644
--- a/src/qt/bitcoin.h
+++ b/src/qt/bitcoin.h
@@ -55,6 +55,10 @@ private:
class BitcoinApplication: public QApplication
{
Q_OBJECT
+
+ // Some wild comment here...
+ int argc{1};
+ const char* argv{"bitcoin-qt"};
public:
explicit BitcoinApplication(interfaces::Node& node);
~BitcoinApplication();
How about this
Results in a segfault.
FWIW, making the argv const and then casting it is how QCoreApplication does it internally when you say argc is 0. The only reason that I did not choose to just do QApplication(0, 0) is because it results in the warning QSettings::value: Empty key passed being printed a couple of times (at least for me using KDE, this might actually just be a KDE thing).
@achow101 But why add the const at all if you're going to cast it away?
Edit: oh, because of the string constant. I get it, the string is read-only, which is fine, Qt is not going to change the contents of arguments, but the array needs to be writable as it might remove arguments.
ACK a2714a5c69f0b0506689af04c3e785f71ee0915d
FWIW, making the
argvconst and then casting it is howQCoreApplicationdoes it internally when you sayargcis 0.
Indeed.
ACK a2714a5c69f0b0506689af04c3e785f71ee0915d
ACK a2714a5c69f0b0506689af04c3e785f71ee0915d - Have tested that arguments like -reverse are no longer being passed through and result in an error.
Some of these options are useful...
Examples of options users should be free to use/play with:
Some of these (eg, -display) are standard options for GUI applications that we don't independently implement. Future versions of Qt may add more useful options.
This change broke the ability to run GUI tests while seeing actions onscreen:
Without being able to set the platform, it's not really possible to write new interactive tests or debug existing ones. I could try to figure out a workaround, maybe using different styles of argument passing for the test program and main program, or maybe implementing some new mechanism for choosing the qt backend other than the standard one...
But honestly I don't find this PR to be very well motivated to begin with, and I wonder if there would be practical downsides to just reverting it. I don't want to step on anybody's toes, but I don't understand where the motivation for this change originally came from and if there are security concerns other than the non-specific "larger attack surface" one cited in comments.
I'd like to know if it'd be ok to submit a PR reverting this change, or if another approach would be suggested.
What about QT_QPA_PLATFORM or one of the other environment variables?
the non-specific "larger attack surface" one cited in comments
We don't want to be too specific about this yet.
I made an issue about the test_bitcoin-qt -platform cocoa: #17013. I see @ryanofsky already spotted it. A QT_QPA_PLATFORM environment variable works for me.
Disclosure of the vulnerability this PR fixes: https://achow101.com/2021/02/0.18-uri-vuln
A possible followup now that this is public would be to explicitly allow safe Qt arguments instead of flatly disabling all of them.
@achow101 Very nice find and thanks for sharing! Has this one been assigned a CVE yet?
Has this one been assigned a CVE yet?
Milestone
0.19.0