Merge lp:~alexlauni/libunity/blacklist-crash-1029949 into lp:libunity

[Alex Launi]

Fix crash when adding or removing players from the blacklist

Fixes segfault from non-null terminated array being passed to gsettings set_strv

[Łukasz Zemczak (sil2100)]

Built and tested with the test case - no crash or problems visible, no regressions as well. +1 for me. So I approve this in overall.

Just two things 'needing fixing'. First - is the printf necessary? It would be best if it was gone ;)

Second thing - could you actually add a test for this bug in this merge request? You could modify the vala test case that you have attached to the bug earlier. It's necessary that all things that can be actually tested land with tests.

review: Needs Fixing

[Alex Launi (alexlauni)]

Ha. Forgot to clear that out. Whoops. fiXXXed.

As for the test. A lot of libunity's test suite has been disabled. I don't have the time to fix the test suite, Ronoc said he'd find some time, but I've added a test so it'll be ready when ronoc fixes the suite.

[Łukasz Zemczak (sil2100)]

LGTM.

review: Approve

[Łukasz Zemczak (sil2100)]

Actually wait, I tried running the test (compiled with --enable-integration-tests=yes) ./test-mpris-backend and it seems to crash somehow. When I comment out the this.player.menu_player.is_blacklisted = true; line the test goes on fine. So I think maybe the test is a bit broken somewhere.

review: Needs Fixing

[Łukasz Zemczak (sil2100)]

I think this is as good as it can get currently, without going the painful path of fixing the integration tests in libunity in overall. Those are still badly broken.

As I already poked you personally about, could we just quickly hack the autogen.sh to add this new test to be executed on make check? Would be most awesome.

[Łukasz Zemczak (sil2100)]

Tested, works awesomely. Good to see you have found that other place causing the same bug, excellent work!
+1

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
No commit message was specified.

review: Needs Fixing (continuous-integration)

=== modified file 'src/unity-sound-menu-mpris.vala'
--- src/unity-sound-menu-mpris.vala	2012-09-20 19:52:53 +0000
+++ src/unity-sound-menu-mpris.vala	2012-10-30 09:13:23 +0000
@@ -376,30 +376,29 @@
     }
     
     private void add_to_blacklist ()
-    {      
-      var already_bl = this.settings.get_strv ("blacklisted-media-players");
-      var new_bl = new ArrayList<string>();
-
-      foreach (var s in already_bl){
-        if ( s == get_blacklist_name()) return;
-        Trace.log_object (this, "add_to_blacklist: %s", s);        
-        new_bl.add(s);
+    {
+      var blacklist = new GLib.VariantBuilder (new VariantType("as"));
+      foreach (var already_blacklisted in this.settings.get_strv("blacklisted-media-players"))
+      {
+        if (already_blacklisted == get_blacklist_name ())
+          return;
+        blacklist.add("s", already_blacklisted);
       }
-      new_bl.add (get_blacklist_name());
-      this.settings.set_strv ("blacklisted-media-players",
-                               new_bl.to_array());
+      blacklist.add("s", get_blacklist_name());
+      
+      this.settings.set_value("blacklisted-media-players", blacklist.end());
     }
 
-    private void remove_from_blacklist ()
+    private void remove_from_blacklist()
     {
-      var already_bl = this.settings.get_strv ("blacklisted-media-players");
-      var new_bl = new ArrayList<string>();
-      foreach (string s in already_bl){
-        if ( s == get_blacklist_name() )continue;
-        new_bl.add(s);
+      var blacklist = new GLib.VariantBuilder(new VariantType("as"));
+      foreach (var already_blacklisted in this.settings.get_strv("blacklisted-media-players"))
+      {
+        if (already_blacklisted == get_blacklist_name())
+          continue;
+        blacklist.add(already_blacklisted);
       }
-      this.settings.set_strv( "blacklisted-media-players",
-                               new_bl.to_array());
+      this.settings.set_value("blacklisted-media-players", blacklist.end());
     }
   }
   /****************************************************************************************/

=== modified file 'test/vala/Makefile.am'
--- test/vala/Makefile.am	2012-10-26 14:42:43 +0000
+++ test/vala/Makefile.am	2012-10-30 09:13:23 +0000
@@ -2,57 +2,57 @@
 
 DISTCHECK_CONFIGURE_FLAGS = --enable-integration-tests=no
 
-check_PROGRAMS = test-vala test-lens test-remote-scope
+check_PROGRAMS = test-vala test-lens test-remote-scope test-blacklist-crash
 
 AM_CPPFLAGS = \
-  -I$(top_srcdir) \
-  -I$(top_builddir)/protocol \
-  -I$(top_builddir)/src \
-  -DBUILDDIR=\"$(top_builddir)\" \
-  -DTESTDIR=\"$(top_srcdir)/test\" \
-  -DTESTVALADIR=\"$(top_srcdir)/test/vala\" \
-  -DG_SETTINGS_ENABLE_BACKEND \
-  $(LIBUNITY_CFLAGS) \
-  $(LIBUNITY_LIBS)
+	-I$(top_srcdir) \
+	-I$(top_builddir)/protocol \
+	-I$(top_builddir)/src \
+	-DBUILDDIR=\"$(top_builddir)\" \
+	-DTESTDIR=\"$(top_srcdir)/test\" \
+	-DTESTVALADIR=\"$(top_srcdir)/test/vala\" \
+	-DG_SETTINGS_ENABLE_BACKEND \
+	$(LIBUNITY_CFLAGS) \
+	$(LIBUNITY_LIBS)
 
-if !ENABLE_C_WARNINGS
-  AM_CPPFLAGS += -w
+if ENABLE_C_WARNINGS
+	AM_CPPFLAGS += -w
 endif
 
 if ENABLE_TRACE_LOG
-  AM_CPPFLAGS += -DENABLE_UNITY_TRACE_LOG
+	AM_CPPFLAGS += -DENABLE_UNITY_TRACE_LOG
 endif
 
 AM_VALAFLAGS = \
-  --vapidir=$(top_builddir)/src \
-  --vapidir=$(top_builddir)/protocol \
-  --vapidir=$(top_srcdir)/test/vala \
-  --pkg unity-protocol \
-  --pkg unity-internal \
-  --pkg config \
-  $(LIBUNITY_PACKAGES) \
-  $(MAINTAINER_VALAFLAGS) \
-  $(NULL)
+	--vapidir=$(top_builddir)/src \
+	--vapidir=$(top_builddir)/protocol \
+	--vapidir=$(top_srcdir)/test/vala \
+	--pkg unity-protocol \
+	--pkg unity-internal \
+	--pkg config \
+	$(LIBUNITY_PACKAGES) \
+	$(MAINTAINER_VALAFLAGS) \
+	$(NULL)
 
 test_libs = \
-  $(top_builddir)/src/libunity.la \
-  $(top_builddir)/protocol/libunity-protocol-private.la \
-  $(LIBUNITY_LIBS)
+	$(top_builddir)/src/libunity.la \
+	$(top_builddir)/protocol/libunity-protocol-private.la \
+	$(LIBUNITY_LIBS)
 
-TEST_PROGS += test-vala test-lens
+TEST_PROGS += test-vala test-lens test-blacklist-crash
 
 test_vala_LDADD = $(test_libs)
 
 test_vala_VALASOURCES = \
-  test-appinfo-manager.vala \
-  test-filters.vala \
-  test-io.vala \
-  test-launcher.vala \
-  test-preferences.vala \
-  test-previews.vala \
-  test-scope-signals.vala \
-  test-vala.vala \
-  $(NULL)
+	test-appinfo-manager.vala \
+	test-filters.vala \
+	test-io.vala \
+	test-launcher.vala \
+	test-preferences.vala \
+	test-previews.vala \
+	test-scope-signals.vala \
+	test-vala.vala \
+	$(NULL)
 nodist_test_vala_SOURCES = $(test_vala_VALASOURCES:.vala=.c)
 
 test_lens_LDADD = $(test_libs)
@@ -63,11 +63,16 @@
 test_remote_scope_VALASOURCES = test-remote-scope.vala
 nodist_test_remote_scope_SOURCES = $(test_remote_scope_VALASOURCES:.vala=.c)
 
+test_blacklist_crash_LDADD = $(test_libs)
+test_blacklist_crash_VALASOURCES = blacklist-crash-1029949-test-case.vala
+nodist_test_blacklist_crash_SOURCES = $(test_blacklist_crash_VALASOURCES:.vala=.c)
+
 BUILT_SOURCES = \
-  test-vala.vala.stamp \
-  test-lens.vala.stamp \
-  test-remote-scope.vala.stamp \
-  $(NULL)
+	test-vala.vala.stamp \
+	test-lens.vala.stamp \
+	test-remote-scope.vala.stamp \
+	test-blacklist-crash.vala.stamp \
+	$(NULL)
 
 test-vala.vala.stamp: $(test_vala_VALASOURCES)
 	$(AM_V_GEN)$(VALAC) -C $(AM_VALAFLAGS) $(VALAFLAGS) $^
@@ -81,19 +86,25 @@
 	$(AM_V_GEN)$(VALAC) -C $(AM_VALAFLAGS) $(VALAFLAGS) $^
 	@touch $@
 
+test-blacklist-crash.vala.stamp: $(test_blacklist_crash_VALASOURCES)
+	$(AM_V_GEN)$(VALAC) -C $(AM_VALAFLAGS) $(VALAFLAGS) $^
+	@touch $@
+
 include Makefile.integration_tests
 
 EXTRA_DIST += \
-  $(test_vala_VALASOURCES) \
-  $(test_lens_VALASOURCES) \
-  $(test_remote_scope_VALASOURCES) \
-  config.vapi \
-  $(NULL)
+	$(test_vala_VALASOURCES) \
+	$(test_lens_VALASOURCES) \
+	$(test_remote_scope_VALASOURCES) \
+	$(test_blacklist_crash_VALASOURCES) \
+	config.vapi \
+	$(NULL)
 
 CLEANFILES = \
-  *.stamp \
-  $(test_vala_VALASOURCES:.vala=.c) \
-  $(test_lens_VALASOURCES:.vala=.c) \
-  $(test_remote_scope_VALASOURCES:.vala=.c) \
-  $(NULL)
+	*.stamp \
+	$(test_vala_VALASOURCES:.vala=.c) \
+	$(test_lens_VALASOURCES:.vala=.c) \
+	$(test_remote_scope_VALASOURCES:.vala=.c) \
+	$(test_blacklist_crash_VALASOURCES:.vala=.c) \
+	$(NULL)
 

=== modified file 'test/vala/Makefile.integration_tests'
--- test/vala/Makefile.integration_tests	2012-08-07 12:31:44 +0000
+++ test/vala/Makefile.integration_tests	2012-10-30 09:13:23 +0000
@@ -139,4 +139,3 @@
 test-mpris-backend-prop-updates-server.vala.stamp: $(test_mpris_backend_prop_updates_server_VALASOURCES)
 	$(AM_V_GEN)$(VALAC) -C $(AM_VALAFLAGS) $(VALAFLAGS) $^
 	@touch $@
-

=== added file 'test/vala/blacklist-crash-1029949-test-case.vala'
--- test/vala/blacklist-crash-1029949-test-case.vala	1970-01-01 00:00:00 +0000
+++ test/vala/blacklist-crash-1029949-test-case.vala	2012-10-30 09:13:23 +0000
@@ -0,0 +1,44 @@
+using Gee;
+using Unity;
+
+
+
+int main(string[] args)
+{
+	int i = 0;
+	var loop = new MainLoop();
+	var time = new TimeoutSource(10);
+	MusicPlayer test_player = null;
+
+	time.set_callback(() => {
+		const string desktop_file_name = "rhythmbox.desktop";
+		test_player = new MusicPlayer(desktop_file_name);
+		test_player.export();
+		
+		return false;	
+	});
+	time.attach(loop.get_context());
+
+	var thread_timer = new TimeoutSource(100);
+	thread_timer.set_callback(() => {
+		set_blacklisted(test_player);
+		if (++i < 100)
+			return true;
+		test_player.unexport();
+		loop.quit();
+		return false;
+	});
+	thread_timer.attach(loop.get_context());
+
+    loop.run();
+	
+	return 0;
+}
+
+void* set_blacklisted(MusicPlayer test_player)
+{
+	int raw_blacklist = Random.int_range(0, 2);
+	bool blacklist = (bool) raw_blacklist;	
+	test_player.is_blacklisted = blacklist;
+	return null;
+}

=== modified file 'test/vala/test-mpris-backend-server.vala'
--- test/vala/test-mpris-backend-server.vala	2011-11-25 12:13:04 +0000
+++ test/vala/test-mpris-backend-server.vala	2012-10-30 09:13:23 +0000
@@ -16,6 +16,9 @@
 
   public bool escape ()
   {
+    // this is some cleanup
+    this.player.menu_player.is_blacklisted = false;
+
     this.mainloop.quit ();
     return false;
   }
@@ -73,6 +76,8 @@
     player.menu_player.player_menu = player_root;
 
     player.menu_player.add_playlist (pl2);
+
+    Timeout.add (5, blacklist_self);
     
     //The manual test of the playlistchanged signal, please leave commented
     //Timeout.add (5, change_playlist_name);                                          
@@ -81,6 +86,13 @@
     return false;
   }
 
+  // Regression test to ensure that setting blacklist does not cause a segfault
+  public bool blacklist_self()
+  {
+    this.player.menu_player.is_blacklisted = true;
+    return false;
+  }
+
   // Manual test of the playlist name change signal
   // Can't test this from the other side of dbus since there is no api to change
   // or edit playlists yet ...