From 1d0dddc627b6e406e560d4c2ae71cd5307ee7b2c Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Mon, 3 May 2021 12:51:52 +0200 Subject: [PATCH] Related: #1952776 (Add patch for crash under gs_details_page_refresh_all() (i#1227)) --- ...sh-under-gs_details_page_refresh_all.patch | 312 ++++++++++++++++++ gnome-software.spec | 7 +- 2 files changed, 318 insertions(+), 1 deletion(-) create mode 100644 001-i1227-crash-under-gs_details_page_refresh_all.patch diff --git a/001-i1227-crash-under-gs_details_page_refresh_all.patch b/001-i1227-crash-under-gs_details_page_refresh_all.patch new file mode 100644 index 0000000..4fcaaf5 --- /dev/null +++ b/001-i1227-crash-under-gs_details_page_refresh_all.patch @@ -0,0 +1,312 @@ +From 64a0eaa4a1b1402a0e5926d0c274cfa962589149 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 29 Apr 2021 15:49:01 +0100 +Subject: [PATCH 1/6] gs-app: Drop requirement for set_version_history() to be + non-NULL + +Signed-off-by: Philip Withnall +--- + lib/gs-app.c | 1 - + 1 file changed, 1 deletion(-) + +diff --git a/lib/gs-app.c b/lib/gs-app.c +index e8eb9d48b..2db3e6251 100644 +--- a/lib/gs-app.c ++++ b/lib/gs-app.c +@@ -5365,7 +5365,6 @@ gs_app_set_version_history (GsApp *app, GPtrArray *version_history) + GsAppPrivate *priv = gs_app_get_instance_private (app); + g_autoptr(GMutexLocker) locker = NULL; + g_return_if_fail (GS_IS_APP (app)); +- g_return_if_fail (version_history != NULL); + locker = g_mutex_locker_new (&priv->mutex); + _g_set_ptr_array (&priv->version_history, version_history); + } +-- +GitLab + + +From 3229d3d66347c5cd317d12db9b456f8eaefad678 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 29 Apr 2021 15:45:33 +0100 +Subject: [PATCH 2/6] gs-app: Clarify documentation/annotations for version + history + +This clarifies the documentation for the existing behaviour. + +Signed-off-by: Philip Withnall + +Helps: #1227 +--- + lib/gs-app.c | 9 +++++---- + 1 file changed, 5 insertions(+), 4 deletions(-) + +diff --git a/lib/gs-app.c b/lib/gs-app.c +index 2db3e6251..5ef7e9942 100644 +--- a/lib/gs-app.c ++++ b/lib/gs-app.c +@@ -129,7 +129,7 @@ typedef struct + GsPluginAction pending_action; + GsAppPermissions permissions; + gboolean is_update_downloaded; +- GPtrArray *version_history; /* (element-type AsRelease) */ ++ GPtrArray *version_history; /* (element-type AsRelease) (nullable) (owned) */ + } GsAppPrivate; + + enum { +@@ -5337,7 +5337,8 @@ gs_app_set_update_permissions (GsApp *app, GsAppPermissions update_permissions) + * Gets the list of past releases for an application (including the latest + * one). + * +- * Returns: (element-type AsRelease) (transfer none): a list ++ * Returns: (element-type AsRelease) (transfer none) (nullable): a list, or ++ * %NULL if the version history is not known + * + * Since: 40 + **/ +@@ -5352,8 +5353,8 @@ gs_app_get_version_history (GsApp *app) + /** + * gs_app_set_version_history: + * @app: a #GsApp +- * @version_history: (element-type AsRelease): a set of entries representing +- * the version history ++ * @version_history: (element-type AsRelease) (nullable): a set of entries ++ * representing the version history, or %NULL if none are known + * + * Set the list of past releases for an application (including the latest one). + * +-- +GitLab + + +From 7910276e5116db3f51dc5b088f3eb567e2472f17 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 29 Apr 2021 15:46:39 +0100 +Subject: [PATCH 3/6] gs-app: Squash empty arrays to NULL in + gs_app_set_version_history() +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +This should save a little bit of memory. I have not updated the +documentation to guarantee this behaviour because it’s a little unusual; +simpler to allow callers to continue handling `NULL` and empty array +cases explicitly. + +Signed-off-by: Philip Withnall + +Helps: #1227 +--- + lib/gs-app.c | 4 ++++ + 1 file changed, 4 insertions(+) + +diff --git a/lib/gs-app.c b/lib/gs-app.c +index 5ef7e9942..6c6bb6fe6 100644 +--- a/lib/gs-app.c ++++ b/lib/gs-app.c +@@ -5366,6 +5366,10 @@ gs_app_set_version_history (GsApp *app, GPtrArray *version_history) + GsAppPrivate *priv = gs_app_get_instance_private (app); + g_autoptr(GMutexLocker) locker = NULL; + g_return_if_fail (GS_IS_APP (app)); ++ ++ if (version_history != NULL && version_history->len == 0) ++ version_history = NULL; ++ + locker = g_mutex_locker_new (&priv->mutex); + _g_set_ptr_array (&priv->version_history, version_history); + } +-- +GitLab + + +From ff5d189202788ec84bf690b8ddb184240ae68e2d Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 29 Apr 2021 15:47:40 +0100 +Subject: [PATCH 4/6] gs-app: Fix generic setters to handle NULL arrays +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +The previous implementation of the functions didn’t handle `NULL` +arrays. + +Signed-off-by: Philip Withnall + +Helps: #1227 +--- + lib/gs-app.c | 8 ++++++-- + 1 file changed, 6 insertions(+), 2 deletions(-) + +diff --git a/lib/gs-app.c b/lib/gs-app.c +index 6c6bb6fe6..efbc4756a 100644 +--- a/lib/gs-app.c ++++ b/lib/gs-app.c +@@ -184,9 +184,11 @@ _g_set_ptr_array (GPtrArray **array_ptr, GPtrArray *new_array) + { + if (*array_ptr == new_array) + return FALSE; ++ if (new_array != NULL) ++ g_ptr_array_ref (new_array); + if (*array_ptr != NULL) + g_ptr_array_unref (*array_ptr); +- *array_ptr = g_ptr_array_ref (new_array); ++ *array_ptr = new_array; + return TRUE; + } + +@@ -195,9 +197,11 @@ _g_set_array (GArray **array_ptr, GArray *new_array) + { + if (*array_ptr == new_array) + return FALSE; ++ if (new_array != NULL) ++ g_array_ref (new_array); + if (*array_ptr != NULL) + g_array_unref (*array_ptr); +- *array_ptr = g_array_ref (new_array); ++ *array_ptr = new_array; + return TRUE; + } + +-- +GitLab + + +From a9597a3a9c04e5dd4571c24dba54abd95eb9fa92 Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 29 Apr 2021 15:50:36 +0100 +Subject: [PATCH 5/6] gs-details-page: Explicitly handle empty version history + array +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +This makes the code a bit more robust. + +It’s not actually needed, as the current behaviour of +gs_app_get_version_history() is to either return `NULL` or a non-empty +array — never an empty array. However, guaranteeing that behaviour would +defy precedent in other APIs, and be confusing to document, so just +handle the case of empty arrays in the callers anyway, to make their +expected behaviour clear. + +Signed-off-by: Philip Withnall +--- + src/gs-app-version-history-dialog.c | 2 +- + src/gs-details-page.c | 2 +- + 2 files changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/gs-app-version-history-dialog.c b/src/gs-app-version-history-dialog.c +index 94d55cc0c..5262596a2 100644 +--- a/src/gs-app-version-history-dialog.c ++++ b/src/gs-app-version-history-dialog.c +@@ -34,7 +34,7 @@ populate_version_history (GsAppVersionHistoryDialog *dialog, + gs_container_remove_all (GTK_CONTAINER (dialog->listbox)); + + version_history = gs_app_get_version_history (app); +- if (version_history == NULL) { ++ if (version_history == NULL || version_history->len == 0) { + GtkWidget *row; + row = gs_app_version_history_row_new (); + gs_app_version_history_row_set_info (GS_APP_VERSION_HISTORY_ROW (row), +diff --git a/src/gs-details-page.c b/src/gs-details-page.c +index cf9b4b393..22b38e929 100644 +--- a/src/gs-details-page.c ++++ b/src/gs-details-page.c +@@ -1305,7 +1305,7 @@ gs_details_page_refresh_all (GsDetailsPage *self) + + /* set version history */ + version_history = gs_app_get_version_history (self->app); +- if (version_history == NULL) { ++ if (version_history == NULL || version_history->len == 0) { + const char *version = gs_app_get_version (self->app); + if (version == NULL || *version == '\0') + gtk_widget_set_visible (self->box_version_history_frame, FALSE); +-- +GitLab + + +From bb1cc3a52fca7e98b053f4c9751df28bfd505dec Mon Sep 17 00:00:00 2001 +From: Philip Withnall +Date: Thu, 29 Apr 2021 15:52:43 +0100 +Subject: [PATCH 6/6] gs-app: Change gs_app_get_version_history() to return a + ref +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +It seems the version history can be updated from a separate thread while +refreshing. This can cause crashes if the UI thread queries the version +history, then it’s updated from another thread, then the UI thread +starts iterating over the `GPtrArray` it queried — which has since been +freed. + +Fix that by returning a strong ref to the `GPtrArray`, and doing so +under the app’s mutex. + +Signed-off-by: Philip Withnall + +Fixes: #1227 +--- + lib/gs-app.c | 11 ++++++++--- + src/gs-app-version-history-dialog.c | 2 +- + src/gs-details-page.c | 2 +- + 3 files changed, 10 insertions(+), 5 deletions(-) + +diff --git a/lib/gs-app.c b/lib/gs-app.c +index efbc4756a..f165df562 100644 +--- a/lib/gs-app.c ++++ b/lib/gs-app.c +@@ -5341,17 +5341,22 @@ gs_app_set_update_permissions (GsApp *app, GsAppPermissions update_permissions) + * Gets the list of past releases for an application (including the latest + * one). + * +- * Returns: (element-type AsRelease) (transfer none) (nullable): a list, or ++ * Returns: (element-type AsRelease) (transfer container) (nullable): a list, or + * %NULL if the version history is not known + * +- * Since: 40 ++ * Since: 41 + **/ + GPtrArray * + gs_app_get_version_history (GsApp *app) + { + GsAppPrivate *priv = gs_app_get_instance_private (app); ++ g_autoptr(GMutexLocker) locker = NULL; + g_return_val_if_fail (GS_IS_APP (app), NULL); +- return priv->version_history; ++ ++ locker = g_mutex_locker_new (&priv->mutex); ++ if (priv->version_history == NULL) ++ return NULL; ++ return g_ptr_array_ref (priv->version_history); + } + + /** +diff --git a/src/gs-app-version-history-dialog.c b/src/gs-app-version-history-dialog.c +index 5262596a2..8b61b7c2b 100644 +--- a/src/gs-app-version-history-dialog.c ++++ b/src/gs-app-version-history-dialog.c +@@ -28,7 +28,7 @@ static void + populate_version_history (GsAppVersionHistoryDialog *dialog, + GsApp *app) + { +- GPtrArray *version_history; ++ g_autoptr(GPtrArray) version_history = NULL; + + /* remove previous */ + gs_container_remove_all (GTK_CONTAINER (dialog->listbox)); +diff --git a/src/gs-details-page.c b/src/gs-details-page.c +index 22b38e929..c985f3b06 100644 +--- a/src/gs-details-page.c ++++ b/src/gs-details-page.c +@@ -1191,7 +1191,7 @@ gs_details_page_refresh_all (GsDetailsPage *self) + guint64 user_integration_bf; + gboolean show_support_box = FALSE; + g_autofree gchar *origin = NULL; +- GPtrArray *version_history; ++ g_autoptr(GPtrArray) version_history = NULL; + guint icon_size; + + if (!gs_page_is_active (GS_PAGE (self))) +-- +GitLab + diff --git a/gnome-software.spec b/gnome-software.spec index 1294911..468c683 100644 --- a/gnome-software.spec +++ b/gnome-software.spec @@ -12,13 +12,15 @@ Name: gnome-software Version: 40.1 -Release: 1%{?dist} +Release: 2%{?dist} Summary: A software center for GNOME License: GPLv2+ URL: https://wiki.gnome.org/Apps/Software Source0: https://download.gnome.org/sources/gnome-software/40/%{name}-%{tarball_version}.tar.xz +Patch01: 001-i1227-crash-under-gs_details_page_refresh_all.patch + BuildRequires: appstream-devel >= %{appstream_version} BuildRequires: gcc BuildRequires: gettext @@ -200,6 +202,9 @@ desktop-file-validate %{buildroot}%{_datadir}/applications/*.desktop %{_datadir}/gtk-doc/html/gnome-software %changelog +* Mon May 03 2021 Milan Crha - 40.1-2 +- Related: #1952776 (Add patch for crash under gs_details_page_refresh_all() (i#1227)) + * Mon May 03 2021 Milan Crha - 40.1-1 - Related: #1952776 (Update to 40.1)