Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not pass in command line arguments to QApplication #16578

Merged
merged 1 commit into from Aug 15, 2019

Conversation

@achow101
Copy link
Member

commented Aug 9, 2019

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.
@promag

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

We don't want any of those built in arguments

Care to explain why?

@achow101

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

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.

@DrahtBot DrahtBot added GUI Tests labels Aug 9, 2019
@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

@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 ?.

@fanquake fanquake removed the Tests label Aug 10, 2019
BitcoinApplication::BitcoinApplication(interfaces::Node& node, int &argc, char **argv):
QApplication(argc, argv),
static int qt_argc = 1;
static const char* qt_argv = "bitcoin-qt";

This comment has been minimized.

Copy link
@laanwj

laanwj Aug 10, 2019

Member
  • these can be inside the function scope
  • 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)

This comment has been minimized.

Copy link
@promag

promag Aug 10, 2019

Member

these can be inside the function scope

What function? Constructor?

This comment has been minimized.

Copy link
@promag

promag Aug 10, 2019

Member

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.

This comment has been minimized.

Copy link
@hebasto

hebasto Aug 10, 2019

Member

Please note:

Warning: The data referred to by argc and argv must stay valid for the entire lifetime of the QCoreApplication object.

This comment has been minimized.

Copy link
@achow101

achow101 Aug 10, 2019

Author Member
* 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]

This comment has been minimized.

Copy link
@achow101

achow101 Aug 10, 2019

Author Member

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.

This comment has been minimized.

Copy link
@hebasto

hebasto Aug 10, 2019

Member

GCC complains if it is non const.

Could be:

Suggested change
static const char* qt_argv = "bitcoin-qt";
static char qt_arg[] = "bitcoin-qt";
static char* qt_argv = qt_arg;

?

This comment has been minimized.

Copy link
@laanwj

laanwj Aug 12, 2019

Member

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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Concept ACK

Copy link
Member

left a comment

Concept ACK

BitcoinApplication::BitcoinApplication(interfaces::Node& node, int &argc, char **argv):
QApplication(argc, argv),
static int qt_argc = 1;
static const char* qt_argv = "bitcoin-qt";

This comment has been minimized.

Copy link
@hebasto

hebasto Aug 10, 2019

Member

Please note:

Warning: The data referred to by argc and argv must stay valid for the entire lifetime of the QCoreApplication object.

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

@hebasto that's why the variables are static, it holds just as true for static variables within a function scope

@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Concept ACK

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Concept ACK: smaller attack surface is better.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Concept ACK.
Codewise, I agree with @laanwj that casting const away and passing it to QApplication seems unfortunate.

@promag

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

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();
@achow101

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2019

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

@fanquake fanquake added this to the 0.19.0 milestone Aug 12, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

ACK a2714a5

@hebasto

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Technically, there is a solution without const.

@achow101

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Technically, there is a solution without const.

I'm going to leave it as is. I feel that doing the same thing that Qt does internally is safe and there's no reason that we should not follow suit.

@hebasto

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

FWIW, making the argv const and then casting it is how QCoreApplication does it internally when you say argc is 0.

Indeed.

ACK a2714a5

Copy link
Member

left a comment

ACK a2714a5 - Have tested that arguments like -reverse are no longer being passed through and result in an error.

@fanquake fanquake merged commit a2714a5 into bitcoin:master Aug 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fanquake added a commit that referenced this pull request Aug 15, 2019
a2714a5 Give QApplication dummy arguments (Andrew Chow)

Pull request description:

  QApplication takes the command line arguments and parses them itself for some [built in command line arguments](https://doc.qt.io/qt-5/qapplication.html#QApplication) 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.

ACKs for top commit:
  laanwj:
    ACK a2714a5
  hebasto:
    ACK a2714a5
  fanquake:
    ACK a2714a5 - Have tested that arguments like `-reverse` are no longer being passed through and result in an error.

Tree-SHA512: 983bd948ca6999f895b6662b58c37e33af7ed61fdd600c6b4623febb87ec06a92c66e3b3300783530110cc711902793ef82d751d7f563696c4c3a8416b2b1f51
fanquake added a commit to fanquake/bitcoin that referenced this pull request Aug 15, 2019
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.

Github-Pull: bitcoin#16578
Rebased-From: a2714a5
@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Added for backport in #16617.

@fanquake fanquake referenced this pull request Aug 15, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Aug 17, 2019
…ation

a2714a5 Give QApplication dummy arguments (Andrew Chow)

Pull request description:

  QApplication takes the command line arguments and parses them itself for some [built in command line arguments](https://doc.qt.io/qt-5/qapplication.html#QApplication) 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.

ACKs for top commit:
  laanwj:
    ACK a2714a5
  hebasto:
    ACK a2714a5
  fanquake:
    ACK a2714a5 - Have tested that arguments like `-reverse` are no longer being passed through and result in an error.

Tree-SHA512: 983bd948ca6999f895b6662b58c37e33af7ed61fdd600c6b4623febb87ec06a92c66e3b3300783530110cc711902793ef82d751d7f563696c4c3a8416b2b1f51
@luke-jr

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Some of these options are useful...

@luke-jr

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Examples of options users should be free to use/play with:

  • -style= style, sets the application GUI style. Possible values are motif, windows, and platinum. If you compiled Qt with additional styles or have additional styles as plugins these will be available to the -style command line option.
  • -style style, is the same as listed above.
  • -stylesheet= stylesheet, sets the application styleSheet. The value must be a path to a file that contains the Style Sheet.
  • -reverse, sets the application's layout direction to Qt::RightToLeft
  • -graphicssystem, sets the backend to be used for on-screen widgets and QPixmaps. Available options are raster and opengl.
  • -display display, sets the X display (default is $DISPLAY).
  • -geometry geometry, sets the client geometry of the first window that is shown.
  • -fn or -font font, defines the application font. The font should be specified using an X logical font description. Note that this option is ignored when Qt is built with fontconfig support enabled.
  • -bg or -background color, sets the default background color and an application palette (light and dark shades are calculated).
  • -fg or -foreground color, sets the default foreground color.
  • -btn or -button color, sets the default button color.
  • -visual TrueColor, forces the application to use a TrueColor visual on an 8-bit display.
  • -ncols count, limits the number of colors allocated in the color cube on an 8-bit display, if the application is using the QApplication::ManyColor color specification. If count is 216 then a 6x6x6 color cube is used (i.e. 6 levels of red, 6 of green, and 6 of blue); for other values, a cube approximately proportional to a 2x3x1 cube is used.
  • -cmap, causes the application to install a private color map on an 8-bit display.
  • -im, sets the input method server (equivalent to setting the XMODIFIERS environment variable)
  • -inputstyle, defines how the input is inserted into the given widget, e.g., onTheSpot makes the input appear directly in the widget, while overTheSpot makes the input appear in a box floating over the widget and is not inserted until the editing is done.

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.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Aug 24, 2019
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.

Github-Pull: bitcoin#16578
Rebased-From: a2714a5
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
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.

Github-Pull: bitcoin#16578
Rebased-From: a2714a5
@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

This change broke the ability to run GUI tests while seeing actions onscreen:

// src/qt/test/test_bitcoin-qt -platform xcb # Linux
// src/qt/test/test_bitcoin-qt -platform windows # Windows
// src/qt/test/test_bitcoin-qt -platform cocoa # macOS

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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

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.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Sep 23, 2019
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.

Github-Pull: bitcoin#16578
Rebased-From: a2714a5
@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

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.

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

This also breaks the URI handler on macOS, but only when opening from the command line, not when opening from a browser. See #17025.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.