Skip to content

fix: cast for UBApplication object during construction#1010

Merged
kaamui merged 1 commit intoOpenBoard-org:devfrom
letsfindaway:fix-qapp-cast
Aug 13, 2024
Merged

fix: cast for UBApplication object during construction#1010
kaamui merged 1 commit intoOpenBoard-org:devfrom
letsfindaway:fix-qapp-cast

Conversation

@letsfindaway
Copy link
Copy Markdown
Collaborator

  • avoid to static_cast UBApplication object while constructor is still not completed
  • fixes problem to determine userDirectory before ApplicationName and OrganizationName are defined

This PR fixes some strange problems we recently had with OpenBoard using the wrong user data directory. Especially it helps (or even solves?) the problems described in

- avoid to static_cast UBApplication object while constructor
  is still not completed
- fixes problem to determine userDirectory before ApplicationName
  and OrganizationName are defined
@Vekhir
Copy link
Copy Markdown
Contributor

Vekhir commented Jun 9, 2024

From a cursory grep on UBApplication::app() it seems like the result is most of the time assumed to be non-nullptr without checking.
In the one case where it is checked, returning a nullptr would disable logging to a file, which doesn't look like it's the problem.
If you've checked and it solves #1004, great. From a theoretic point of view (I don't have this issue), this doesn't seem related.

Of course, the issue remains that UBApplication::app() is uninitialised memory. Since everything depends on the app, perhaps just aborting the program might be the safest route. The logging could get a check like UBApplication::isInitialized (using dynamic_cast like here) to make it clear that it could be called during initialisation.

@letsfindaway
Copy link
Copy Markdown
Collaborator Author

letsfindaway commented Jun 9, 2024

From a cursory grep on UBApplication::app() it seems like the result is most of the time assumed to be non-nullptr without checking. In the one case where it is checked, returning a nullptr would disable logging to a file, which doesn't look like it's the problem. If you've checked and it solves #1004, great. From a theoretic point of view (I don't have this issue), this doesn't seem related.

It is related in the following way:
grafik

  • During startup, UBApplication is instantiated.
  • As this inherits from SingleApplication and this again from QApplication these constructors are executed first.
  • During initialization of QApplication a debug message is generated stating "Qt: Session management error: None of the authentication protocols specified are supported". For me this error message occurs since this patch for CVE-2024-36041) was applied to my Plasma5 desktop. This message is what triggers the problem.
  • The debug output is intercepted by ub_message_output().
  • This function tries to determine whether the message must be written to a log file and first calls UBApplication::app()->isVerbose().
  • But because UBApplication's constructor is still not completed, the mIsVerbose variable is not initialized and this function returns an arbitrary value from uninitialized memory.
  • In many cases this is non-null and interpreted as true. Therefore UBSettings::userDataDirectory() is called to determine the log file directory.
  • As this is the first call to this function, dataDirPath is empty.
  • I don't have the "App/DataDirectory" variable and therefore we determine the directory from QStandardPaths::writableLocation(QStandardPaths::AppLocalDataLocation).
  • As setOrganizationName and setApplicationName are still not executed, the name of the executable is used to construct the path. In my case this path ends with an all lowercase openboard.
  • This path is then stored in dataDirPath, which is never changed later.
  • Note that even the getAppSettings() uses a wrong file name for the settings for the same reason.
  • Phew, this is a long chain of events and it took me quite some time to figure it out.

Of course, the issue remains that UBApplication::app() is uninitialised memory. Since everything depends on the app, perhaps just aborting the program might be the safest route. The logging could get a check like UBApplication::isInitialized (using dynamic_cast like here) to make it clear that it could be called during initialisation.

I was thinking about better checks for nullptr but then did not do it for the following reasons:

  • As soon as qApp is initialized, it always points to an instance of UBApplication and the dynamic_cast is always successful.
  • While qApp is completely uninitialized, it contains nullptr. Here a static_cast and a dynamic_cast return the same value, i.e. also a nullptr. But this is only in the few lines from the beginning of main() to the construction of the UBApplication object and there is no usage of qApp.
  • So the only difference in the return value is when UBApplication::app() is called while the object is being constructed. This is never done directly in any code of OpenBoard. The only way I can think for such a call is the indirect invocation from ub_message_output() as explained above. And here the result of the cast is checked.

So I don't think that any further check is necessary.

@Vekhir
Copy link
Copy Markdown
Contributor

Vekhir commented Jun 9, 2024

Thanks for detailing the causal relationship. I agree, this is the best course of action.

This shows how "set at first use" is flawed, but I can't think of anything better.

Copy link
Copy Markdown

@maehne maehne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, as already stated in issue #1004 . Thanks @letsfindaway for the detailed analysis of the problem's cause!

@kaamui kaamui merged commit b78e87e into OpenBoard-org:dev Aug 13, 2024
@letsfindaway letsfindaway deleted the fix-qapp-cast branch August 13, 2024 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants