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.

Sign up for GitHub

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

Jump to bottom

SIGSEGV when opening a file #3151

Closed
overcq opened this issue Apr 7, 2022 · 7 comments · Fixed by #3156
Closed

SIGSEGV when opening a file #3151

overcq opened this issue Apr 7, 2022 · 7 comments · Fixed by #3156
Milestone
2.0

Comments

@overcq
Copy link

overcq commented Apr 7, 2022

Starting from commit be739e2 geany crashes with SIGSEGV when I try to open a file.
I'm using Gentoo Linux (updated).
Working commit da12533 configuration: configure.1.txt
Not working commit be739e2 configuration: configure.2.txt
gdb geany and open from dialog: backtrace.2-dialog.txt
gdb geany and open from File Browser plugin: backtrace.2-plugin.txt
Working commit strace geany Program.cs: strace.1.txt
Not working commit strace geany Program.cs: strace.2-args.txt
@eht16
Copy link
Member

eht16 commented Apr 7, 2022

Most probably the same cause as in #3149.

Please note that even if the backtrace puts vte_send_cmd() in a prominent position, the crash there is only a symptom of the underlying problem that vc->send_cmd_prefix is NULL because its fallback value is not applied when read from the config.

@overcq as a workaround, the crashes should stop if you disable the 'Follow path of current file' option in VTE preferences.

@eht16 eht16 mentioned this issue Apr 7, 2022
Merged
@kugel- kugel- mentioned this issue Apr 7, 2022
Closed
@overcq
Copy link
Author

overcq commented Apr 8, 2022

The problem was the new location of the call for the settings_action function, which was before setting up stash groups in the load_dialog_prefs function.
There is patch file: solution.txt

@eht16
Copy link
Member

eht16 commented Apr 9, 2022

The patch seems to fix the issue, in my case it was vc->send_cmd_prefix not being read properly (it remained NULL) and with the patch it is read from the config and initialised to an empty string (which is correct).

@kugel- I'm not familiar with the "stash" system, could you have a look if the fix is appropriate? Thanks.

@kugel-
Copy link
Member

kugel- commented Apr 9, 2022

Alright, I think I understand. It's the specific "VTE" stash group that is initialized later (now after settions_action()). I missed that because all other stash groups are created init_pref_groups().

The patch probably works but I would rather explore why the "VTE" stash group needs to be special, that's just unexpected and surprising. Hopefully I can manage in the coming days.

@elextr
Copy link
Member

elextr commented Apr 9, 2022

why the "VTE" stash group needs to be special, that's just unexpected and surprising

Is it because loading VTE can be disabled by pref, so at least that pref needs to be initialised and loaded from the file and tested to see if the VteConfig struct needs to be created, and the stash group can't be created before the VteConfig struct because it's members (eg send_command_prefix) point to members in that struct so they would be dangling pointers if the group was created when VteConfig wasn't.

@elextr
Copy link
Member

elextr commented Apr 9, 2022

Thinking some more, can VteConfig be always created (ie be static) if HAVE_VTE is defined? Its only 18 members, and even if alignment made them all pointers its inconsequential.

kugel- added a commit to kugel-/geany that referenced this issue Apr 10, 2022
@kugel-
Setup VTE stash group in init_pref_groups()
b09dd1a 
Like with all other stash groups, the VTE one is now set up in
init_pref_groups(), along with the terminal various pref group.

Since be739e2 ("session.conf split follow-up #3"), the send_cmd_prefix
pref was read from the configuration before setting up the stash group,
which caused the "Follow path of the current file" feature to crash
Geany. I.e. the fix is to set up the stash group even earlier.

In my optionion, it's also beneficial that the overall stash groups
do not depend on loading libvte or not. For example, previously the
terminal various pref group was only added when loading libvte was not
disabled. When it was disabled the end result was inconsistent with
the manual. Now the terminal group appears unconditionally.

Fixes be739e2 ("session.conf split follow-up #3")
Fixes geany#3151
@kugel- kugel- mentioned this issue Apr 10, 2022
Merged
@kugel-
Copy link
Member

kugel- commented Apr 10, 2022

@overcq thanks for your investigation. I decided to follow a different path for the fix, basically what @elextr suggests as well. See #3156

kugel- added a commit to kugel-/geany that referenced this issue Apr 10, 2022
@kugel-
Setup VTE stash group in init_pref_groups()
8c80a17 
Like with all other stash groups, the VTE one is now set up in
init_pref_groups(), along with the terminal various pref group.

Since be739e2 ("session.conf split follow-up #3"), the send_cmd_prefix
pref was read from the configuration before setting up the stash group,
which caused the "Follow path of the current file" feature to crash
Geany. I.e. the fix is to set up the stash group even earlier.

In my optionion, it's also beneficial that the overall stash groups
do not depend on loading libvte or not. For example, previously the
terminal various pref group was only added when loading libvte was not
disabled. When it was disabled the end result was inconsistent with
the manual. Now the terminal group appears unconditionally.

Fixes be739e2 ("session.conf split follow-up #3")
Fixes geany#3151
kugel- added a commit to kugel-/geany that referenced this issue Apr 11, 2022
@kugel-
Setup VTE stash group in init_pref_groups() (PR geany#3156)
8662ed2 
Like with all other stash groups, the VTE one is now set up in
init_pref_groups(), along with the terminal various pref group.

Since be739e2 ("session.conf split follow-up #3"), the send_cmd_prefix
pref was read from the configuration before setting up the stash group,
which caused the "Follow path of the current file" feature to crash
Geany. I.e. the fix is to set up the stash group even earlier.

In my optionion, it's also beneficial that the overall stash groups
do not depend on loading libvte or not. For example, previously the
terminal various pref group was only added when loading libvte was not
disabled. When it was disabled the end result was inconsistent with
the manual. Now the terminal group appears unconditionally.

Fixes be739e2 ("session.conf split follow-up #3")
Fixes geany#3151
kugel- added a commit that referenced this issue Apr 11, 2022
@kugel-
Setup VTE stash group in init_pref_groups() (PR #3156)
df27d1b 
Like with all other stash groups, the VTE one is now set up in
init_pref_groups(), along with the terminal various pref group.

Since be739e2 ("session.conf split follow-up #3"), the send_cmd_prefix
pref was read from the configuration before setting up the stash group,
which caused the "Follow path of the current file" feature to crash
Geany. I.e. the fix is to set up the stash group even earlier.

In my optionion, it's also beneficial that the overall stash groups
do not depend on loading libvte or not. For example, previously the
terminal various pref group was only added when loading libvte was not
disabled. When it was disabled the end result was inconsistent with
the manual. Now the terminal group appears unconditionally.

Fixes be739e2 ("session.conf split follow-up #3")
Fixes #3151
@b4n b4n added this to the 1.39/2.0 milestone Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0
Development

Successfully merging a pull request may close this issue.

Unconditionally set up VTE stash group (fixes #3151) kugel-/geany
5 participants
@kugel- @eht16 @b4n @elextr @overcq