Jamulus iOS crashes on Qt 5.15.19, likely due to stack allocation in main.cpp

[ann0see]

Describe the bug

Jamulus iOS crashed on my local Qt 5.15.19 build. I was fighting to debug it and then asked GPT 5 SOL. It was extremely fast (and expensive) and pointed out that we allocate too much on the stack causing corruption leading to overwrite something in Qt. I think it does indeed make sense to change this.
Especially the Client object is likely too large.

Also we should remove src/iOS/ios_app_delegate.mm/.h probably as it pollutes some functions in Qt.

Unfortunately, I could not follow the thinking output and cleared the session too quickly - the crash got fixed.

Suggestion: we should use std::unique_ptr's and heap allocation. Maybe this also fixes the Qt6 crashes.

The produced fix actually works.
Proposed diff:

diff --git a/src/main.cpp b/src/main.cpp
index a518c2e5..1250d7bf 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -980,17 +980,18 @@ int main ( int argc, char** argv )
 #ifndef SERVER_ONLY
         if ( bIsClient )
         {
-            CClient Client ( iPortNumber, iQosNumber, bNoAutoJackConnect, strClientName, bDisableIPv6, bMuteMeInPersonalMix );
+            std::unique_ptr<CClient> Client (
+                new CClient ( iPortNumber, iQosNumber, bNoAutoJackConnect, strClientName, bDisableIPv6, bMuteMeInPersonalMix ) );
 
             // Create Settings with the client pointer
-            CClientSettings Settings ( &Client, strIniFileName );
+            CClientSettings Settings ( Client.get(), strIniFileName );
             Settings.Load ( CommandLineOptions );
-            Client.SetSettings ( &Settings );
+            Client->SetSettings ( &Settings );
 
 #    ifndef NO_JSON_RPC
             if ( pRpcServer )
             {
-                new CClientRpc ( &Client, &Settings, pRpcServer, pRpcServer );
+                new CClientRpc ( Client.get(), &Settings, pRpcServer, pRpcServer );
             }
 #    endif
 
@@ -1006,7 +1007,7 @@ int main ( int argc, char** argv )
 
                 // GUI object
                 CClientDlg
-                    ClientDlg ( &Client, &Settings, strConnOnStartupAddress, bShowComplRegConnList, bShowAnalyzerConsole, bMuteStream, nullptr );
+                    ClientDlg ( Client.get(), &Settings, strConnOnStartupAddress, bShowComplRegConnList, bShowAnalyzerConsole, bMuteStream, nullptr );
 
                 // show dialog
                 ClientDlg.show();
@@ -1018,8 +1019,8 @@ int main ( int argc, char** argv )
             {
                 if ( !strConnOnStartupAddress.isEmpty() )
                 {
-                    Client.SetServerAddr ( strConnOnStartupAddress );
-                    Client.Start();
+                    Client->SetServerAddr ( strConnOnStartupAddress );
+                    Client->Start();
                 }
 
                 // only start application without using the GUI
@@ -1033,32 +1034,32 @@ int main ( int argc, char** argv )
         {
             // Server:
             // actual server object
-            CServer Server ( iNumServerChannels,
-                             strLoggingFileName,
-                             strServerBindIP4,
-                             strServerBindIP6,
-                             iPortNumber,
-                             iQosNumber,
-                             strDirectoryAddress,
-                             strServerListFileName,
-                             strServerInfo,
-                             strServerPublicIP,
-                             strServerListFilter,
-                             strWelcomeMessage,
-                             strRecordingDirName,
-                             bDisconnectAllClientsOnQuit,
-                             bUseDoubleSystemFrameSize,
-                             bDisableRaw,
-                             bUseMultithreading,
-                             bDisableRecording,
-                             bDelayPan,
-                             bDisableIPv6,
-                             eLicenceType );
+            std::unique_ptr<CServer> Server ( new CServer ( iNumServerChannels,
+                                                            strLoggingFileName,
+                                                            strServerBindIP4,
+                                                            strServerBindIP6,
+                                                            iPortNumber,
+                                                            iQosNumber,
+                                                            strDirectoryAddress,
+                                                            strServerListFileName,
+                                                            strServerInfo,
+                                                            strServerPublicIP,
+                                                            strServerListFilter,
+                                                            strWelcomeMessage,
+                                                            strRecordingDirName,
+                                                            bDisconnectAllClientsOnQuit,
+                                                            bUseDoubleSystemFrameSize,
+                                                            bDisableRaw,
+                                                            bUseMultithreading,
+                                                            bDisableRecording,
+                                                            bDelayPan,
+                                                            bDisableIPv6,
+                                                            eLicenceType ) );
 
 #ifndef NO_JSON_RPC
             if ( pRpcServer )
             {
-                new CServerRpc ( &Server, pRpcServer, pRpcServer );
+                new CServerRpc ( Server.get(), pRpcServer, pRpcServer );
             }
 
 #endif
@@ -1066,7 +1067,7 @@ int main ( int argc, char** argv )
             if ( bUseGUI )
             {
                 // load settings from init-file (command line options override)
-                CServerSettings Settings ( &Server, strIniFileName );
+                CServerSettings Settings ( Server.get(), strIniFileName );
                 Settings.Load ( CommandLineOptions );
 
                 // load translation
@@ -1076,7 +1077,7 @@ int main ( int argc, char** argv )
                 }
 
                 // GUI object for the server
-                CServerDlg ServerDlg ( &Server, &Settings, bStartMinimized, nullptr );
+                CServerDlg ServerDlg ( Server.get(), &Settings, bStartMinimized, nullptr );
 
                 // show dialog (if not the minimized flag is set)
                 if ( !bStartMinimized )
@@ -1096,7 +1097,7 @@ int main ( int argc, char** argv )
                 // strDirectoryAddress is wanted
                 if ( !strDirectoryAddress.isEmpty() )
                 {
-                    Server.SetDirectoryType ( AT_CUSTOM );
+                    Server->SetDirectoryType ( AT_CUSTOM );
                 }
 
                 pApp->exec();

To Reproduce

Compile Qt 5.15.19 on macOS, build Jamulus.
Expected behavior

Jamulus does not crash
Screenshots

/
Operating system

Version of Jamulus

Additional context

[ann0see]

As far as I know, smart pointers auto free?

[softins]

As far as I know, smart pointers auto free?

Yes: https://en.cppreference.com/cpp/memory/unique_ptr - whatever is pointed to is assumed to be delete-able when the smart pointer goes out of scope.

This looks good. For consistency, in main(), should we also do the same for CClientSettings, CClientDlg, CServerSettings and CServerDlg?

[softins]

Also, the pointers returned from new CClientRpc() and new CServerRpc() are not stored anywhere, so those objects remain until program termination. Each should be assigned to a std::unique_ptr, but that should be allocated outside of the if() { }, or the object would die immediately.

[softins]

Jamulus iOS crashed on my local Qt 5.15.19 build. I was fighting to debug it and then asked GPT 5 SOL. It was extremely fast (and expensive) and pointed out that we allocate too much on the stack causing corruption leading to overwrite something in Qt. I think it does indeed make sense to change this. Especially the Client object is likely too large.

The Client doesn't seem very large. It's actually the Server object that is huge:

For Client:

sizeof(Client) = 7760
sizeof(Settings) = 224
sizeof(ClientDlg) = 1788

For Server:

sizeof(Server) = 241336
sizeof(Settings) = 24
sizeof(ServerDlg) = 320

So having the unique_ptr just for Server and Client would probably be enough.

Did your iOS build crash as a client? And did this unique_ptr change fix that?

[ann0see]

Yes, it crashed as client. However reverting this fix no longer makes the crash reproducible. So it could also be something else that GPT fixed silently which is invisible in the code and just something related to my Qt build. I do not see anything important that it changed though unfortunately.

[pljones]

My only concern with this is Qt thread management. Qt understands QObject& and makes sure it's thread-safe when passed in signals. I don't know if that applies to std::unique_ptr<QObject> references -- or, indeed, whether it matter for these references.

[softins]

My only concern with this is Qt thread management. Qt understands QObject& and makes sure it's thread-safe when passed in signals. I don't know if that applies to std::unique_ptr<QObject> references -- or, indeed, whether it matter for these references.

It doesn't matter, as it's only to do with object lifetime. Instead of allocating a whole object on the stack (such as Client or Server, we allocate it on the heap with new, and store the returned pointer in the std::unique_ptr<T>, which itself is allocated on the stack. We can still pass the object's own pointer around as at present, as uses of it are not controlling the object's lifetime; only the unique_ptr is doing that.

So instead of CClient Client(args);, we have std::unique_ptr<CClient> Client (new CClient(args)); in the same place. And when the latter Client goes out of scope (at the same place where the former Client did), the CClient object on the heap will be destroyed and freed with delete automatically.

And instead of passing &Client as an argument to a function or object constructor that expects a CClient* pClient, we would pass Client.get(), which would still be a CClient* as expected by the called function. There is no need to pass the whole unique_ptr to called functions.

Finally, within the scope of the unique_ptr declaration, any uses that were previously Client.member or Client.function() would become Client->member or Client->function().

You can see these changes in @ann0see's proposed diff above.

[pljones]

OK, so for these references it's fine. Is it safe as a pattern for handling references within a Qt multi-(Q)threaded application in general? What I'm trying to get at is whether we need to put some clear guidelines in place as to when to apply it.