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

Fix qt crash #502

Closed
wants to merge 2 commits into from
Closed

Fix qt crash #502

wants to merge 2 commits into from

Conversation

Cimbali
Copy link
Contributor

@Cimbali Cimbali commented Oct 23, 2024

This PR fixes #500 by:

  • adding the import that defines ColorImage (an undocumented QtQuick internal) to the files that use it. The better fix would be to use a documented way of displaying images, but this is the simplest fix.
  • Renames popupType to popupPrio as it is used for priority of popups, and because popupType is a (final) property of QtQuick popups since 6.8

Note that:

  • Fixes are trivial
  • Any platforms running Qt 6.8 are affected, so not Arch-specific as claimed in Fails to start with Qt 6.8 #500 (neither OpenSUSE-specific, Ubuntu non-LTS, Fedora 41, Fedora Rawhide, etc. etc. -- all up-to-date platforms are affected by the bug, the others will be in time when updating Qt)
  • Qt 6.8 is marked as LTS so there is even more value in fixing this early on
  • Seen the dismissive answers on Fails to start with Qt 6.8 #500 😢, I share this more for package maintainers and self-builders than hoping it will get merged. Feel free to use these patches until upstream provides their own fixes.

@Antiz96
Copy link

Antiz96 commented Oct 24, 2024

@Cimbali Thanks a lot!

I can confirm these patches work as intended (tested on Arch Linux). I backported them to the Arch's protonmail-bridge package while waiting for them to be merged & released here (or for upstream to provide their own fixes at some point).

@joekm
Copy link

joekm commented Oct 24, 2024

Thanks for the patch. Also felt bad for the developers getting all that whining from a few Arch users over such a trivial matter. I. E. - just run it without the QT interface until 6.8 becomes more commonplace. This kind of thing is "par for the course" for Arch (or any distro close to "bleeding edge"), and the Proton developers had an easy work around already in place.

@gabor-meszaros
Copy link
Collaborator

gabor-meszaros commented Oct 28, 2024

Hi @Cimbali, Thank you for your contribution. We have reviewed and tested your changes and we're pleased to announce that they've been approved. We will perform QA testing on all our supported platforms, and if the results are positive, your contribution will be included in our next release.

Note that this GitHub repository is a mirror of our internal development repository, so this pull request cannot be approved and merged here. We will cherry pick your two commits into our repository, and this pull request will be closed. Obviously, you will get credit for your work as the authorship of the commit is preserved in the process.

@jaredmo
Copy link

jaredmo commented Oct 29, 2024

Thanks @Cimbali for this patch, and @Antiz96 for implementing so quickly in Arch.

@singatias
Copy link

Thanks for the patch. Also felt bad for the developers getting all that whining from a few Arch users over such a trivial matter. I. E. - just run it without the QT interface until 6.8 becomes more commonplace. This kind of thing is "par for the course" for Arch (or any distro close to "bleeding edge"), and the Proton developers had an easy work around already in place.

I would be too if the response they gave make sense but it actually bring me more worries than anything else as a proton business user, anyway not trying to start new drama but I was really unpleased with the official answers on that thread.

@thereillywriter
Copy link

Thanks for the patch. Also felt bad for the developers getting all that whining from a few Arch users over such a trivial matter. I. E. - just run it without the QT interface until 6.8 becomes more commonplace. This kind of thing is "par for the course" for Arch (or any distro close to "bleeding edge"), and the Proton developers had an easy work around already in place.

I would be too if the response they gave make sense but it actually bring me more worries than anything else as a proton business user, anyway not trying to start new drama but I was really unpleased with the official answers on that thread.

Indeed. As a Proton Business user, I am going to run Bridge in headless or CLI mode going forward. Relying on EOL dependencies is not a good development practice and the official responses did not acknowledge this at all. It is nice of Arch Linux to backport these fixes to their version of the Bridge packages, but upstream should really be looking after these issues. That is what we, the customers, pay them to do after all. While I donate to Arch Linux from time to time, they are not contractually obliged to support my business (and I do not expect them to). As a business user, I expect Proton to step in at times like this and do the needful.

For now, I am tied into a contract with Proton but I will certainly be looking at migrating to alternatives which more consistently value security in their development practices. I accept that no business is perfect all of the time, but the slow response and dismissive attitude from Proton is extremely disappointing and worrying and speaks to wider issues about their management and culture.

@Cimbali
Copy link
Contributor Author

Cimbali commented Oct 30, 2024

Please, @joekm @singatias @thereillywriter, none of this is constructive.

Yes, using up-to-date dependencies would be nice. Yes, running bleeding edge distros means occasional unsupported dependencies and bugs. But developer time is finite.

This PR is opened in the spirit of constructively sharing solutions, and ranting only gets discussions closed and reduces the space for people to collaborate. If you’re unhappy with proton, with how they prioritise their resources, or with other users’ opinions (?), send them an email or write a post (including X, reddit, blogs -- whatever your favourite platform may be). If you agree or disagree with something said, use a reaction emoji. But github is not a place to vent. And forcing technical staff to spend time moderating discussions hardly seems a better use of resources.

@singatias
Copy link

Please, @joekm @singatias @thereillywriter, none of this is constructive.

Yes, using up-to-date dependencies would be nice. Yes, running bleeding edge distros means occasional unsupported dependencies and bugs. But developer time is finite.

This PR is opened in the spirit of constructively sharing solutions, and ranting only gets discussions closed and reduces the space for people to collaborate. If you’re unhappy with proton, with how they prioritise their resources, or with other users’ opinions (?), send them an email or write a post (including X, reddit, blogs -- whatever your favourite platform may be). If you agree or disagree with something said, use a reaction emoji. But github is not a place to vent. And forcing technical staff to spend time moderating discussions hardly seems a better use of resources.

Hi @Cimbali, I’ll quickly explain why this issue bothered me, but I won’t be discussing it further here. The issue has nothing to do with Arch Linux or using bleeding-edge libraries/features. The real issue is with the following response that was given:

Qt is only used for the user interface that is well separated from the part of Bridge that is security sensitive (they are different processes), therefore using an EOL Qt version doesn't make the application insecure.

From a security standpoint, this is quite concerning and gives the impression of lack of knowledge on the subject and “we don’t care about it”.

Attackers often target UI libraries with known vulnerabilities, as they can still open indirect access to data or resources, depending on the application’s design. If an attacker gains control over the UI process, they could exploit or manipulate inter-process communications, depending on the security of the protocol. If the UI has access to sensitive resources, even indirectly, this could be a potential attack vector.

As a developer, I consider it poor practice not to update dependencies to an LTS version, especially for a company claiming to prioritize privacy and security.

I'm not asking for having it fix in the next 24 hours but at least have it added to your backlog as a potential security issue.

@joekm
Copy link

joekm commented Nov 1, 2024

I stand behind my response. This program is run-able without the QT interface and, if you're running a bleeding edge distro, that's to be expected. This is not a security issue.

@Cimbali Cimbali closed this Nov 13, 2024
V3n3RiX pushed a commit to redcorelinux/redcore-desktop that referenced this pull request Dec 5, 2024
V3n3RiX pushed a commit to redcorelinux/redcore-desktop that referenced this pull request Dec 5, 2024
	* import from Gentoo && version bump
	* fix GUI start-up failure with qt6.8
	* ProtonMail/proton-bridge#500
	* ProtonMail/proton-bridge#502
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.

Fails to start with Qt 6.8
7 participants