fix: potential crash caused by dlopen different gtk libraries by deepak1556 · Pull Request #33650 · electron/electron

deepak1556 · 2022-04-07T12:12:36Z

Description of Change

#19159 refactored file dialog implementation that relies requiring gtk library at runtime, around the same time chromium has been refactoring itself to also require gtk at runtime https://bugs.chromium.org/p/chromium/issues/detail?id=1192861

Since chromium already loads the required gtk library into the process on startup, the file dialog implementation should just get a handle to the loaded library and perform operations on it, the current implementation can lead to situations in classic snaps where two different version gtk libraries can get loaded into the same process triggering a crash, refs microsoft/vscode#146584 (comment)

NB: There is no functionality change in this PR, simplifies library loading following upstream changes and fixes potential crash alongside.

/cc @tristan957

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: fix crash when opening gtk file dialogs due to mismatched versions

tristan957 · 2022-04-07T14:46:21Z

Hi @deepak1556. Thanks for tracking down the source of the issue. With your PR, I am a little confused how any of the gtk_file_chooser_native_* methods work because it is possible that those symbols don't exist.

Where is the definition of electron::IsElectron_gtkInitialized()?

deepak1556 · 2022-04-07T15:02:48Z

electron_gtk_stubs.cc is responsible for loading the symbols and providing utilities to check if the required symbols are loaded, this file is generated during compile time. An example can be seen with upstream's stub file that gets generated for https://source.chromium.org/chromium/chromium/src/+/main:ui/gtk/gtk.sig, if the symbol is not present it will just call the utility function (example) is provided as part of logging_function in the Build.gn file. In our case, we provide a noop logger which allows for symbol load failures.

tristan957 · 2022-04-07T15:12:03Z

Ok cool. Thanks for the background. This PR sounds like a good solution then. I wonder if Flatpak has the same issue.

deepak1556 · 2022-04-07T16:01:27Z

Not familiar with flaptak, but if it does not search for libraries from the host then worst case would be it fails to load the required gtk version with the symbols and end up calling the legacy dialog fallback. I think only classical snaps can run into this crash edge case.

tristan957 · 2022-04-07T16:04:34Z

I am not at all familiar with the confinement of snap vs classical snap vs flatpak. As you said, I guess classical snap is the outlier here and it is really weird to have to program around this packaging format when seemingly it would work for snap and flatpak.

nornagon · 2022-04-07T16:57:19Z

+Subject: Make gtk::GetLibGtk public
+
+Allows embedders to get a handle to the gtk library
+already loaded in the process.


should this be upstreamed?

I can give it an attempt.

release-clerk · 2022-04-18T04:24:35Z

Release Notes Persisted

fix crash when opening gtk file dialogs due to mismatched versions

trop · 2022-04-18T04:25:08Z

I was unable to backport this PR to "16-x-y" cleanly;
you will need to perform this backport manually.

trop · 2022-04-18T04:25:14Z

I was unable to backport this PR to "17-x-y" cleanly;
you will need to perform this backport manually.

trop · 2022-04-18T04:25:33Z

I have automatically backported this PR to "19-x-y", please check out #33813

…on#33650)

trop · 2022-06-28T15:54:12Z

@miniak has manually backported this PR to "17-x-y", please check out #34774

fix: potential crash caused by dlopen different gtk libraries (#33650)

…on#33650)

fix: potential crash caused by dlopen different gtk libraries

56c568e

deepak1556 requested review from a team as code owners April 7, 2022 12:12

electron-cation Bot added the new-pr 🌱 PR opened recently label Apr 7, 2022

deepak1556 requested review from ckerr and zcbenz April 7, 2022 12:12

deepak1556 added semver/patch backwards-compatible bug fixes target/16-x-y labels Apr 7, 2022

nornagon reviewed Apr 7, 2022

View reviewed changes

Comment thread shell/browser/ui/electron_gtk.sigs

nornagon reviewed Apr 7, 2022

View reviewed changes

Comment thread shell/browser/electron_browser_main_parts.cc

nornagon approved these changes Apr 7, 2022

View reviewed changes

electron-cation Bot removed the new-pr 🌱 PR opened recently label Apr 8, 2022

zcbenz approved these changes Apr 10, 2022

View reviewed changes

zcbenz merged commit 37b7e34 into main Apr 18, 2022

zcbenz deleted the robo/fix_gtk_loading branch April 18, 2022 04:24

trop Bot added needs-manual-bp/16-x-y and removed target/16-x-y labels Apr 18, 2022

trop Bot mentioned this pull request Apr 18, 2022

fix: potential crash caused by dlopen different gtk libraries #33812

Merged

trop Bot added in-flight/18-x-y and removed target/18-x-y labels Apr 18, 2022

bavulapati pushed a commit to bavulapati/electron that referenced this pull request Apr 29, 2022

fix: potential crash caused by dlopen different gtk libraries (electr…

b11ed31

…on#33650)

rzhao271 mentioned this pull request May 4, 2022

refactor: use stubs for gdk-pixbuf dependency #34077

Merged

5 tasks

deermichel mentioned this pull request May 6, 2022

[bug, linux] crash on start undefined symbol: gtk_native_dialog_get_type #34118

Closed

3 tasks

shoffmeister mentioned this pull request May 19, 2022

[Bug]: Improve Linux XDG FileChooser implementation #31258

Closed

3 tasks

deepak1556 mentioned this pull request Jun 6, 2022

chore: update rpm deps microsoft/vscode#151348

Merged

ghost mentioned this pull request Jun 21, 2022

[pull] main from electron:main Blaquez-home/electron#3

Open

miniak added a commit that referenced this pull request Jun 28, 2022

fix: potential crash caused by dlopen different gtk libraries (#33650)

5331dec

miniak added a commit that referenced this pull request Jun 28, 2022

fix: potential crash caused by dlopen different gtk libraries (#33650)

84e5b11

miniak added a commit that referenced this pull request Jun 28, 2022

fix: potential crash caused by dlopen different gtk libraries (#33650)

3f574c3

miniak added a commit that referenced this pull request Jun 28, 2022

fix: potential crash caused by dlopen different gtk libraries (#33650)

036a2d7

miniak mentioned this pull request Jun 28, 2022

fix: potential crash caused by dlopen different gtk libraries #34774

Merged

trop Bot added in-flight/17-x-y and removed needs-manual-bp/17-x-y labels Jun 28, 2022

miniak added a commit that referenced this pull request Jun 28, 2022

fix: potential crash caused by dlopen different gtk libraries (#33650)

6889188

miniak added a commit that referenced this pull request Jun 28, 2022

fix: potential crash caused by dlopen different gtk libraries (#33650)

3c0ea37

miniak added a commit that referenced this pull request Jun 30, 2022

fix: potential crash caused by dlopen different gtk libraries (#33650)

6f31ae2

trop Bot added merged/17-x-y and removed in-flight/17-x-y labels Jun 30, 2022

VerteDinde pushed a commit that referenced this pull request Jun 30, 2022

fix: potential crash caused by dlopen different gtk libraries (#34774)

342eb67

fix: potential crash caused by dlopen different gtk libraries (#33650)

khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023

fix: potential crash caused by dlopen different gtk libraries (electr…

23c82ec

…on#33650)

Navigation Menu

Uh oh!

fix: potential crash caused by dlopen different gtk libraries - #33650

fix: potential crash caused by dlopen different gtk libraries#33650
zcbenz merged 1 commit into
mainfrom
robo/fix_gtk_loading

deepak1556 commented Apr 7, 2022 •

edited

Loading

Uh oh!

tristan957 commented Apr 7, 2022

Uh oh!

deepak1556 commented Apr 7, 2022 •

edited

Loading

Uh oh!

tristan957 commented Apr 7, 2022

Uh oh!

deepak1556 commented Apr 7, 2022

Uh oh!

tristan957 commented Apr 7, 2022

Uh oh!

Uh oh!

nornagon Apr 7, 2022

Uh oh!

deepak1556 Apr 8, 2022

Uh oh!

Uh oh!

release-clerk Bot commented Apr 18, 2022

Uh oh!

trop Bot commented Apr 18, 2022

Uh oh!

trop Bot commented Apr 18, 2022

Uh oh!

trop Bot commented Apr 18, 2022

Uh oh!

trop Bot commented Jun 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

deepak1556 commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Checklist

Release Notes

Uh oh!

tristan957 commented Apr 7, 2022

Uh oh!

deepak1556 commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tristan957 commented Apr 7, 2022

Uh oh!

deepak1556 commented Apr 7, 2022

Uh oh!

tristan957 commented Apr 7, 2022

Uh oh!

Uh oh!

nornagon Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

deepak1556 Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

release-clerk Bot commented Apr 18, 2022

Uh oh!

trop Bot commented Apr 18, 2022

Uh oh!

trop Bot commented Apr 18, 2022

Uh oh!

trop Bot commented Apr 18, 2022

Uh oh!

trop Bot commented Jun 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deepak1556 commented Apr 7, 2022 •

edited

Loading

deepak1556 commented Apr 7, 2022 •

edited

Loading