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