gonetz / GLideN64

A new generation, open-source graphics plugin for N64 emulators.
Other
754 stars 174 forks source link

GLideNUI/ConfigDialog: Qt issue with argc/argv #2813

Closed scurest closed 3 months ago

scurest commented 5 months ago

Look at this code in GLideNUI.cpp

https://github.com/gonetz/GLideN64/blob/3b43a13a80dfc2eb6357673440b335e54eaa3896/src/GLideNUI/GLideNUI.cpp#L34-L42

Consulting the documentation for QApplication::QApplication(int &argc, char **argv), we can read

Warning: The data referred to by argc and argv must stay valid for the entire lifetime of the QApplication object. In addition, argc must be greater than zero and argv must contain at least one valid character string.

Note: argc and argv might be changed as Qt removes command line arguments that it recognizes.

This code violates all three conditions.

I don't know anything about Qt, but if that if-block is supposed to run only once, then a simple fix would be just to use statics

diff --git a/src/GLideNUI/GLideNUI.cpp b/src/GLideNUI/GLideNUI.cpp
index 1f324520..43a7cc92 100644
--- a/src/GLideNUI/GLideNUI.cpp
+++ b/src/GLideNUI/GLideNUI.cpp
@@ -35,9 +35,10 @@ int openConfigDialog(const wchar_t * _strFileName, const wchar_t * _strSharedFil
    QCoreApplication* pApp = QCoreApplication::instance();

    if (pApp == nullptr) {
-       int argc = 0;
-       char * argv = 0;
-       pQApp.reset(new QApplication(argc, &argv));
+       static int argc = 1;
+       static char argv0[] = "GLideN64";
+       static char * argv[1] = { argv0 };
+       pQApp.reset(new QApplication(argc, argv));
        pApp = pQApp.get();
    }

Context

I'm fairly sure this is the cause of certain non-deterministic crashes I get when opening the Config Dialog while working on a branch. The code gets to w.show(), which eventually calls QCoreApplication::arguments(), which crashes in strlen... usually. The above patch appears to fix it.

Unfortunately I wasn't able to repro with master, and I wasn't able to get far enough with -fsanitize=address to reach that line, so I don't have a reproduction for you, but hopefully the doc violation will be enough to convince you it's worth changing.

Rosalie241 commented 3 months ago

Thank you for noticing that issue and pointing to the documentation, your suggested code looks fine, can you submit a PR?

scurest commented 3 months ago

There's code in openAboutDialog with the same problem

https://github.com/gonetz/GLideN64/blob/525ce82318002fb6d47bbea9bfa00a35a0c3deea/src/GLideNUI/GLideNUI.cpp#L60-L68

I don't really understand why this code isn't guarded by QCoreApplication::instance == nullptr the same as the config code? Also how would I open the About dialog in RMG to test it?

Rosalie241 commented 3 months ago

in mupen64plus, that function isn't used, only in Project64