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.
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?
static BitcoinApplication::create() where you could declare a 
* 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.
0warning: 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:
0static char qt_arg[] = "bitcoin-qt";
1static 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.
How about this
0diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
1index 5ce4f3c19..f38235b15 100644
2--- a/src/qt/bitcoin.cpp
3+++ b/src/qt/bitcoin.cpp
4@@ -169,11 +169,8 @@ void BitcoinCore::shutdown()
5 }
6 }
7
8-static int qt_argc = 1;
9-static const char* qt_argv = "bitcoin-qt";
10-
11 BitcoinApplication::BitcoinApplication(interfaces::Node& node):
12- QApplication(qt_argc, const_cast<char **>(&qt_argv)),
13+ QApplication(argc, const_cast<char **>(&argv)),
14 coreThread(nullptr),
15 m_node(node),
16 optionsModel(nullptr),
17diff --git a/src/qt/bitcoin.h b/src/qt/bitcoin.h
18index 3869193a3..d8b7f3b36 100644
19--- a/src/qt/bitcoin.h
20+++ b/src/qt/bitcoin.h
21@@ -55,6 +55,10 @@ private:
22 class BitcoinApplication: public QApplication
23 {
24 Q_OBJECT
25+
26+ // Some wild comment here...
27+ int argc{1};
28+ const char* argv{"bitcoin-qt"};
29 public:
30 explicit BitcoinApplication(interfaces::Node& node);
31 ~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.
FWIW, making the
argvconst and then casting it is howQCoreApplicationdoes it internally when you sayargcis 0.
Indeed.
ACK a2714a5c69f0b0506689af04c3e785f71ee0915d
-reverse are no longer being passed through and result in an error.
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.
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.
Has this one been assigned a CVE yet?