qt5: some programs' tests try to delete /Applications

[RJVB (René Bertin)]

Last week I ran into a dangerous bug in Qt5, as a result of which I had to restore a large part of /Applications from backup.

The bug is in QtCore::QStandardPaths (QSP) which provides applications with a way to obtain the paths to a range of standard locations, in readable ("system") and writable ("user") form. The class also has a switch to put it into testing mode that changes the returned locations, intended for use in unittests/autotests so that they don't overwrite or delete anything in the actual locations.

On OS X, QSP returns locations that are compliant with the OS X way of doing things (app. data into ~/Library/Application Support, for instance) and App Store rules.

For the ApplicationsLocation it returns /Applications, which seems reasonable (but in fact has nothing to do with the role of the ApplicationsLocation on Linux). This location makes no difference between readable and writable, and more importantly, not either between regular and testing mode.

As a result, software tests that verify functionality involving reading and writing from ApplicationsLocation will

• attempt to get the testing ApplicationsLocation
• store files there, do stuff with them
• cleanup afterwards

The code that wreaked havoc on my machine did the cleanup with a removeDir(ApplicationsLocation), in other words, rm -rf /Applications. Evidently it didn't print exactly what it was doing, it just seemed to hang (fortunately I have an HDD, not an SDD). (And of course I have since reverified all permissions in /Applications.)

I reported the bug upstreams, but at the moment any fixing appears to be blocked by a single Qt dev, and a fix isn't likely to be included anytime soon anyway (5.7, maybe).

I did include a fix in the QSP patch of my own port:qt5-kde, of course.

I did run into this problem while developing a port, but I don't think there is much risk with published ports. port test probably doesn't run with elevated privileges, and port maintainers should just check any (auto)tests or keep/set test.run no.

However, anyone using the installed Qt5 SDK should be aware of the risk.

[mkae (Marko Käning)]

Cc Me!

[MarcusCalhoun-Lopez (Marcus Calhoun-Lopez)]

Replying to rjvbertin@…:

I reported the bug upstreams, but at the moment any fixing appears to be blocked by a single Qt dev, and a fix isn't likely to be included anytime soon anyway (5.7, maybe).

Is there a link on ​https://bugreports.qt.io?

I did include a fix in the QSP patch of my own port:qt5-kde, of course.

Could you please say which patch?

[RJVB (René Bertin)]

Replying to mcalhoun@…:

Is there a link on ​https://bugreports.qt.io?

No, this has been discussed on the mailing lists only.

I did include a fix in the QSP patch of my own port:qt5-kde, of course.

Could you please say which patch?

It has the evocative name fix_qstandardpaths3.patch .

[RJVB (René Bertin)]

BTW, there's a companion patch called fix-qsp_fontlocations.patch that addresses a related issue in the FontsLocation. That one is separate because I did report it, and it's supposed to be fixed in 5.7 .

[MarcusCalhoun-Lopez (Marcus Calhoun-Lopez)]

For reference:

• The issue was first brought up on the ​KDE mailing list.
• It then proceeded to the ​QT mailing list.
• A possible solution can be found in the ​zip file of #48967 or the ​git repository.

If I understand the problem:

• If isTestModeEnabled() returns true, ​standard paths change.
  • /Library/Application Support becomes ~/.qttest/Application Support.
  • etc.
• On OS X, ApplicationsLocation stays /Applications whether isTestModeEnabled() return true or not.
  
• There is a program out there that runs removeDir(ApplicationsLocation) assuming that it is deleting a folder in ~/.qttest
• This assumption has catastrophic results on OS X.

isTestModeEnabled() return true, what is ApplicationsLocation on Windows and Linux that would make removeDir(ApplicationsLocation) safe?
Doesn't removeDir(ApplicationsLocation) delete ALL other application testing data and not just for that particular program?
Are you simply not supposed to run two tests at the same time?

This sounds like a problem that needs to be addressed either upstream or with the unit test writers.
I would suggest submitting a bug or a patch with either group.

[RJVB (René Bertin)]

Replying to mcalhoun@…:

• There is a program out there that runs removeDir(ApplicationsLocation) assuming that it is deleting a folder in ~/.qttest
• This assumption has catastrophic results on OS X.

There could be any number of such (test) programmes. Part of my current routine in writing KF5 ports is to verify all auto-tests for dangerous behaviour, and replace the port test settings with a warning when I get a hit (and KF5 is being built without my QSP patch).

isTestModeEnabled() return true, what is ApplicationsLocation on Windows and Linux that would make removeDir(ApplicationsLocation) safe?

No idea on MS Windows (look at the relevant Qt code...) but on Linux we're talking about $HOME/.local/share/applications vs $HOME/.qttest/share/applications. Also, ApplicationsLocation doesn't contain the actual applications but only files with metadata pertaining to applications.

Doesn't removeDir(ApplicationsLocation) delete ALL other application testing data and not just for that particular program?
Are you simply not supposed to run two tests at the same time?

Yes, and yes I guess. Of course if you do, and test b removes stuff test a requires, all that happens is that test a fails. You'll then try test a again, and it'll succeed ... and you'll end up understanding that you're seeing expected behaviour.

This sounds like a problem that needs to be addressed either upstream or with the unit test writers.
I would suggest submitting a bug or a patch with either group.

As I said before, Qt is aware, and a fix will appear, but probably later rather than sooner. I also don't know if the issue will address the fact that ApplicationsLocation does not have the same meaning across platforms. That is something that will have to be solved independently. Unit test writers are not doing anything wrong, the bug is not theirs, and without documentation saying that ApplicationsLocation should not be used on OS X they have no reason to take that into consideration.

The plan is to re-submit my QSP patch to Qt at some future time when it will have had sufficient real-world testing; changes to QStandardPaths as well as the activator principle to toggle QSP from "native" into "XDG-compliant" mode.

[RJVB (René Bertin)]

Replying to rjvbertin@…:

(and KF5 is being built without my QSP patch).

In my current approach, the QSP/XDG activator gets linked to all KF5 frameworks if the +QspXDG variant is set.

• That variant is the default if the activator is detected locally (but can of course be overridden from the commandline, using -QspXDG)
• This is the case if port:qt5-kde is installed
• The KF5 frameworks indicate a preference for port:qt5-kde
• That means that port:qt5-kde will be installed when starting from a clean slate like on the build bots.

Thus, subports providing individual KF5 frameworks can simply check if +QspXDG is set; if not, test.cmd and test.args can be set so that a warning is printed.

Ports can also use ${qt_bins_dir}/qtpath --testmode --writable-path ApplicationsLocation to check the actual location used, but that is not going to inform whether about QSP's operating mode during the tests.

[MarcusCalhoun-Lopez (Marcus Calhoun-Lopez)]

For such a substantial ​patch, I think that the best course of action is to first submit it to ​codereview.qt-project.org.
If it gets merged upstream, then we can incorporate it our repository.

[RJVB (René Bertin)]

You don't get it...

The patch is indeed substantial, and that's the very reason why it won't be considered upstream until there's real-world evidence to show that it has been tested and works as advertised, without side-effects etc.

And once it is incorporated there's no more use to incorporate it in MacPorts ...

[MarcusCalhoun-Lopez (Marcus Calhoun-Lopez)]

Replying to rjvbertin@…:

You don't get it...

I do appreciate you taking the time to patiently explain it.

The patch is indeed substantial, and that's the very reason why it won't be considered upstream until there's real-world evidence to show that it has been tested and works as advertised, without side-effects etc.

I am a full supporter of cutting edge experimentation but only in some sort of -devel port.
Personally, I use Qt for my work and need at least one non-experimental version around.

And once it is incorporated there's no more use to incorporate it in MacPorts ...

Perhaps this is the point of confusion.
When I say merged, I mean merged into the git repository that Qt uses for development.
I do not mean released.
If you look at certain changes (​140876, ​125968, ​127759 to name just a few), they have been merged but not released.
In fact, it may be quite some time (months) before they are released, but since they have been vetted and approved by upstream developers, they are excellent candidates for patchfiles.
For those types of changes, there is indeed great use in incorporating them into MacPorts.
In fact, Qt will not fully build on El Capitan without at least ​one of them.

[RJVB (René Bertin)]

Replying to mcalhoun@…:

I am a full supporter of cutting edge experimentation but only in some sort of -devel port.
Personally, I use Qt for my work and need at least one non-experimental version around.

Well, that's just one more argument to have a qt5-kde port, and for the approach I'm taking with the patch in question. If you read my earlier explanation, it doesn't change anything if you don't activate the new code-paths it provides. That is also a prerequisite for getting it accepted upstream, because Apple won't accept applications using XDG-compliant locations in their App Store.

Since you mention it: does it matter for your work whether Qt applications use ~/Library/Preferences and ~/Library/Application Support or rather ~/.config and ~/.local/share ? It's the possibility to support both options that makes my patch so complicated (and hard to defend without being able to refer to real-world use cases).

Perhaps this is the point of confusion.
When I say merged, I mean merged into the git repository that Qt uses for development.
I do not mean released.

Oh, I know that. The point remains that Qt's vetting process is one I only engage in if I'm more or less certain that my proposition will be accepted. And when that proposition isn't changing or intersecting with something else I'm working on, because their "gerrit" is *really* annoying in that case.

If you look at certain changes (​140876, ​125968, ​127759 to name just a few), they have been merged but not released.
In fact, it may be quite some time (months) before they are released, but since they have been vetted and approved by upstream developers, they are excellent candidates for patchfiles.
For those types of changes, there is indeed great use in incorporating them into MacPorts.
In fact, Qt will not fully build on El Capitan without at least ​one of them.

[RJVB (René Bertin)]

One more thing: the development version we're talking about is 5.7 . That's *two* "minor" releases away from the current, and in addition the code in question has been rewritten to remove the use of Carbon APIs. In other words, even if my patch gets accepted it will not apply "as is" to older versions.

[RJVB (René Bertin)]

You don't think that's a dangerous bug?

[mkae (Marko Käning)]

+1

I think it is a dangerous bug!

[mkae (Marko Käning)]

How to proceed with this, guys?

[RJVB (René Bertin)]

port:qt5-kde will fix this along with the other required modifications to QStandardPaths, and I made upstream aware of it via a bug report.

There probably is little risk for regular Qt5 ports (how many include auto/unit tests anyway?) and I deactivate the test feature in the KF5 ports if they are built against port:qt5 instead of port:qt5-kde .

[MarcusCalhoun-Lopez (Marcus Calhoun-Lopez)]

I could not find the upstream bug report.
Has this been merged yet?

[RJVB (René Bertin)]

No, I never managed to convince them about this. There *may* have been a change that reduces the risk of actually deleting /Applications (by adding ~/Applications as the writable path so you only lose your own personal apps) but I continue to patch QSP to behave in a more appropriate fashion.

​https://bugreports.qt.io/browse/QTBUG-44346 ​https://codereview.qt-project.org/#/c/154229/

[MarcusCalhoun-Lopez (Marcus Calhoun-Lopez)]

So should we keep this ticket open?

[RJVB (René Bertin)]

I think I don't care either way. If I feel the need I can always reopen or refile.

Applies to the other ticket too.