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

VTE: Fix crash when "send_cmd_prefix" is NULL #3153

Merged
merged 1 commit into from May 22, 2022

Conversation

eht16
Copy link
Member

@eht16 eht16 commented Apr 7, 2022

Related to #3151 and #3149.

The NULL check in vte_send_cmd() is just for feeling more safe, the main change is to consider vc->send_cmd_prefix being NULL when constructing the command to be executed when we want to change the directory in the VTE.
This should not happen but it might.

@eht16 eht16 added the vte Issues relating to the builtin terminal emulator (VTE) label Apr 7, 2022
@kugel-
Copy link
Member

kugel- commented Apr 10, 2022

Obsoleted by #3156 (IMO)

@frlan
Copy link
Member

frlan commented Apr 11, 2022

Having this patch active I didn't see further crashes hitting e.g. f5 most likely caused by in thix context unset send_cmd_prefix

@eht16
Copy link
Member Author

eht16 commented Apr 11, 2022

Obsoleted by #3156 (IMO)

Not necessarily I'd say.
I think the guards here are useful anyway. True, it should not happen that vte_config.send_cmd_prefix is NULL and that any caller should pass NULL to vte_send_cmd but still it might happen and Geany should not crash then.

@kugel-
Copy link
Member

kugel- commented Apr 11, 2022

But we aren't going to augment every pref in case garbage is read from geany.conf, do we?

@eht16
Copy link
Member Author

eht16 commented Apr 11, 2022

No, of course not.
But since we now know that vte_config.send_cmd_prefix might lead to crashes when it is NULL for whatever reasons, checking it makes it not worse, IMO.

@kugel-
Copy link
Member

kugel- commented Apr 30, 2022

I'm not blocking, go ahead if you think it's worthwhile

@elextr
Copy link
Member

elextr commented May 2, 2022

@eht16 ping, still complaints about this problem, see #3190

@eht16
Copy link
Member Author

eht16 commented May 15, 2022

Just fixed the merge conflict. If there are no objections, let's merge it.

@elextr
Copy link
Member

elextr commented May 15, 2022

LGTM

@eht16 eht16 merged commit 66b28eb into geany:master May 22, 2022
@eht16 eht16 deleted the vte_handle_null_command branch May 22, 2022 12:54
@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
Labels
vte Issues relating to the builtin terminal emulator (VTE)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants