Bug 779924 – gnome-games crash with SIGSEGV in retro_core_set_callbacks()

After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.

Bug 779924 - gnome-games crash with SIGSEGV in retro_core_set_callbacks()


Summary:	gnome-games crash with SIGSEGV in retro_core_set_callbacks()


Status:	RESOLVED FIXED

Product:	gnome-games
Classification:	Core
Component:	general
Version:	unspecified
Hardware:	Other Linux

Importance:	Normal normal
Target Milestone:	---
Assigned To:	GNOME Games maintainers
QA Contact:	GNOME Games maintainers

URL:
Whiteboard:

Depends on:
Blocks:

Reported:	2017-03-12 02:42 UTC by Jeremy Bicha
Modified:	2017-03-20 13:36 UTC

See Also:
GNOME target:	---
GNOME version:	---

Attachments
retro: Check if module file exists before getting its path (1.50 KB, patch) 2017-03-13 11:45 UTC, Abhinav Singh	none	Details \| Review
core-descriptor: Check if module file exists in get_module_file() (906 bytes, patch) 2017-03-13 11:46 UTC, Abhinav Singh	none	Details \| Review
core-descriptor: Check existence of module file in get_module_file() (1.05 KB, patch) 2017-03-14 16:41 UTC, Abhinav Singh	committed	Details \| Review
retro: Check existence of module file (1.62 KB, patch) 2017-03-14 18:49 UTC, Abhinav Singh	committed	Details \| Review

Description Jeremy Bicha 2017-03-12 02:42:23 UTC

gnome-games 3.23.91
locally built on Ubuntu 17.04 Beta

If you have /usr/lib/x86_64-linux-gnu/libretro/nestopia.libretro but don't have nestopia installed and click on a .nes game in GNOME Games, GNOME Games will crash.

This probably won't happen unless there's a packaging bug, right? But maybe it shouldn't crash anyway.

Comment 1 Jeremy Bicha 2017-03-12 02:43:32 UTC

+ Trace 237244

#0 ??
#1 retro_core_set_callbacks
from /usr/lib/x86_64-linux-gnu/libretro-gtk.so.0
#2 ??
from /usr/lib/x86_64-linux-gnu/libretro-gtk.so.0
#3 ??
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#4 g_object_new_valist
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#5 g_object_new
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#6 ??
#7 ??
#8 games_application_window_run_game
#9 g_closure_invoke
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#10 ??
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#11 g_signal_emit_valist
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#12 g_signal_emit_by_name
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#13 g_closure_invoke
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#14 ??
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#15 g_signal_emit_valist
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#16 g_signal_emit_by_name
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#17 g_closure_invoke
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#18 ??
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#19 g_signal_emit_valist
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#20 g_signal_emit
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#21 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#22 ffi_call_unix64
from /usr/lib/x86_64-linux-gnu/libffi.so.6
#23 ffi_call
from /usr/lib/x86_64-linux-gnu/libffi.so.6
#24 g_cclosure_marshal_generic_va
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#25 ??
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#26 g_signal_emit_valist
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#27 g_signal_emit
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#28 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#29 g_cclosure_marshal_VOID__BOXEDv
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#30 ??
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#31 g_signal_emit_valist
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#32 g_signal_emit
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#33 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#34 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#35 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#36 gtk_event_controller_handle_event
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#37 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#38 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#39 ??
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#40 g_signal_emit_valist
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#41 g_signal_emit
from /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#42 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#43 ??
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#44 gtk_main_do_event
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#45 ??
from /usr/lib/x86_64-linux-gnu/libgdk-3.so.0
#46 ??
from /usr/lib/x86_64-linux-gnu/libgdk-3.so.0
#47 g_main_context_dispatch
from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#48 ??
from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#49 g_main_context_iteration
from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#50 g_application_run
from /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0
#51 _vala_main
#52 __libc_start_main
at ../csu/libc-start.c line 291
#53 _start

Comment 2 Abhinav Singh 2017-03-13 11:45:12 UTC

Created attachment 347821 [details] [review]
retro: Check if module file exists before getting its path

Comment 3 Abhinav Singh 2017-03-13 11:46:20 UTC

Created attachment 347822 [details] [review]
core-descriptor: Check if module file exists in get_module_file()

Comment 4 Adrien Plazas 2017-03-14 09:03:31 UTC

Review of attachment 347822 [details] [review]:

::: retro-gtk/retro-core-descriptor.vala
@@ +93,1 @@
+		return module_file.query_exists () ? module_file : null;

Looks good to me: either we have a File object pointing to a file, or null. :)

I would rather split this though, for readability's sake and to better blend into the rest of the codebase:
var module = get_module ();
var module_file = dir.get_child (module);
if (!module_file.query_exists ())
	return null;

return module_file;

Comment 5 Adrien Plazas 2017-03-14 09:08:57 UTC

Review of attachment 347822 [details] [review]:

Oh also, it would be nice to have an explanation of the motivation for this change in the log.

Comment 6 Adrien Plazas 2017-03-14 09:37:32 UTC

Review of attachment 347821 [details] [review]:

The shortlog is a tiny bit long, maybe "retro: Check existence of module file" and then detail what is done in the rest of the log.

::: src/retro/retro-runner.vala
@@ +260,1 @@
 			module_path = module_file.get_path ();

A small nitpick: a newline would be nice there, it would help readability after the "throw" line which breaks the execution flow.

Comment 7 Adrien Plazas 2017-03-14 09:38:48 UTC

Jeremy: do these patches (one for retro-gtk, the other for gnome-games) fix your problem?

Comment 8 Abhinav Singh 2017-03-14 16:41:33 UTC

Created attachment 347929 [details] [review]
core-descriptor: Check existence of module file in get_module_file()

File.get_child(string name) returns a File even if no file with such
name exists. Hence, check if this File points to anything else return
null.

Comment 9 Abhinav Singh 2017-03-14 18:49:21 UTC

Created attachment 347939 [details] [review]
retro: Check existence of module file

Retro.CoreDescriptor.get_module_file() returns null when module file is
not found. Null check module file before getting its path for Retro.Core.

Comment 10 Jeremy Bicha 2017-03-14 22:47:01 UTC

Yes, the patches (347929 and 347939) fix my issue. Thanks!

Comment 11 Adrien Plazas 2017-03-17 13:50:48 UTC

Review of attachment 347929 [details] [review]:

Thanks! :)

Comment 12 Adrien Plazas 2017-03-17 13:51:50 UTC

Review of attachment 347939 [details] [review]:

Thanks!

Comment 13 Adrien Plazas 2017-03-17 14:08:22 UTC

Comment on attachment 347929 [details] [review]
core-descriptor: Check existence of module file in get_module_file()

Attachment 347929 [details] pushed as c5dc74c - core-descriptor: Check existence of module file in get_module_file()

Comment 14 Adrien Plazas 2017-03-20 13:36:40 UTC

Attachment 347939 [details] pushed as 431a74e - retro: Check existence of module file