From 71ef7c7eedef2d2d8ec8dce0c576ba972edd775e Mon Sep 17 00:00:00 2001 From: Sebastian Ramacher Date: Sun, 19 Jan 2014 17:09:33 +0100 Subject: [PATCH] Fix some memory leaks and a use after free Also return proper D-Bus errors Signed-off-by: Sebastian Ramacher --- database-plain.c | 38 ++++++++++++++++++++------------------ database-sqlite.c | 14 +++++++++++--- dbus-interface.c | 38 +++++++++++++++++++++++++++++++++++--- plugin.c | 5 ++++- render.c | 16 +++++++++------- utils.c | 7 +++---- zathura.c | 6 +++--- 7 files changed, 85 insertions(+), 39 deletions(-) diff --git a/database-plain.c b/database-plain.c index e1f00ef..1825af9 100644 --- a/database-plain.c +++ b/database-plain.c @@ -326,26 +326,22 @@ plain_add_bookmark(zathura_database_t* db, const char* file, return false; } - char* bmx = g_try_malloc(G_ASCII_DTOSTR_BUF_SIZE); - if (bmx == NULL) { - return false; - } - - char* bmy = g_try_malloc(G_ASCII_DTOSTR_BUF_SIZE); - if (bmy == NULL) { - g_free(bmx); - return false; - } - char* name = prepare_filename(file); char* val_list[] = { g_strdup_printf("%d", bookmark->page), - g_ascii_dtostr(bmx, G_ASCII_DTOSTR_BUF_SIZE, bookmark->x), - g_ascii_dtostr(bmy, G_ASCII_DTOSTR_BUF_SIZE, bookmark->y) + g_try_malloc0(G_ASCII_DTOSTR_BUF_SIZE), + g_try_malloc0(G_ASCII_DTOSTR_BUF_SIZE) }; + if (name == NULL || val_list[1] == NULL || val_list[2] == NULL) { + g_free(name); + for (unsigned int i = 0; i < LENGTH(val_list); ++i) { + g_free(val_list[i]); + } + return false; + } - g_free(bmx); - g_free(bmy); + g_ascii_dtostr(val_list[1], G_ASCII_DTOSTR_BUF_SIZE, bookmark->x); + g_ascii_dtostr(val_list[2], G_ASCII_DTOSTR_BUF_SIZE, bookmark->y); g_key_file_set_string_list(priv->bookmarks, name, bookmark->id, (const char**)val_list, LENGTH(val_list)); @@ -417,7 +413,15 @@ plain_load_bookmarks(zathura_database_t* db, const char* file) } bookmark->id = g_strdup(keys[i]); - char **val_list = g_key_file_get_string_list(priv->bookmarks, name, keys[i], &num_vals, NULL); + char** val_list = g_key_file_get_string_list(priv->bookmarks, name, keys[i], + &num_vals, NULL); + + if (num_vals != 1 && num_vals != 3) { + girara_error("Unexpected number of values."); + g_free(bookmark); + g_strfreev(val_list); + continue; + } bookmark->page = atoi(val_list[0]); @@ -427,8 +431,6 @@ plain_load_bookmarks(zathura_database_t* db, const char* file) } else if (num_vals == 1) { bookmark->x = DBL_MIN; bookmark->y = DBL_MIN; - } else { - girara_debug("This must be a BUG"); } girara_list_append(result, bookmark); diff --git a/database-sqlite.c b/database-sqlite.c index 9525759..b3696c3 100644 --- a/database-sqlite.c +++ b/database-sqlite.c @@ -388,6 +388,10 @@ sqlite_load_bookmarks(zathura_database_t* db, const char* file) girara_list_t* result = girara_sorted_list_new2((girara_compare_function_t) zathura_bookmarks_compare, (girara_free_function_t) zathura_bookmark_free); + if (result != NULL) { + sqlite3_finalize(stmt); + return NULL; + } while (sqlite3_step(stmt) == SQLITE_ROW) { zathura_bookmark_t* bookmark = g_try_malloc0(sizeof(zathura_bookmark_t)); @@ -397,8 +401,8 @@ sqlite_load_bookmarks(zathura_database_t* db, const char* file) bookmark->id = g_strdup((const char*) sqlite3_column_text(stmt, 0)); bookmark->page = sqlite3_column_int(stmt, 1); - bookmark->x = sqlite3_column_double(stmt, 2); - bookmark->y = sqlite3_column_double(stmt, 3); + bookmark->x = sqlite3_column_double(stmt, 2); + bookmark->y = sqlite3_column_double(stmt, 3); if (bookmark->page > 1) { bookmark->x = bookmark->x == 0.0 ? DBL_MIN : bookmark->x; @@ -524,8 +528,12 @@ sqlite_load_jumplist(zathura_database_t* db, const char* file) } girara_list_t* jumplist = girara_list_new2(g_free); - int res = 0; + if (jumplist == NULL) { + sqlite3_finalize(stmt); + return NULL; + } + int res = 0; while ((res = sqlite3_step(stmt)) == SQLITE_ROW) { zathura_jump_t* jump = g_try_malloc0(sizeof(zathura_jump_t)); if (jump == NULL) { diff --git a/dbus-interface.c b/dbus-interface.c index 7b8ab05..b2cf9c2 100644 --- a/dbus-interface.c +++ b/dbus-interface.c @@ -291,18 +291,40 @@ handle_method_call(GDBusConnection* UNUSED(connection), /* get rectangles */ girara_list_t** rectangles = g_try_malloc0(number_of_pages * sizeof(girara_list_t*)); if (rectangles == NULL) { + g_variant_iter_free(iter); + g_variant_iter_free(secondary_iter); + g_dbus_method_invocation_return_error(invocation, G_DBUS_ERROR, + G_DBUS_ERROR_NO_MEMORY, + "Failed to allocate memory."); return; } rectangles[page] = girara_list_new2(g_free); + if (rectangles[page] == NULL) { + g_free(rectangles); + g_variant_iter_free(iter); + g_variant_iter_free(secondary_iter); + g_dbus_method_invocation_return_error(invocation, G_DBUS_ERROR, + G_DBUS_ERROR_NO_MEMORY, + "Failed to allocate memory."); + return; + } zathura_rectangle_t temp_rect; while (g_variant_iter_loop(iter, "(dddd)", &temp_rect.x1, &temp_rect.x2, &temp_rect.y1, &temp_rect.y2)) { zathura_rectangle_t* rect = g_try_malloc0(sizeof(zathura_rectangle_t)); if (rect == NULL) { - continue; + g_variant_iter_free(iter); + g_variant_iter_free(secondary_iter); + girara_list_free(rectangles[page]); + g_free(rectangles); + g_dbus_method_invocation_return_error(invocation, G_DBUS_ERROR, + G_DBUS_ERROR_NO_MEMORY, + "Failed to allocate memory."); + return; } + *rect = temp_rect; girara_list_append(rectangles[page], rect); } @@ -322,9 +344,19 @@ handle_method_call(GDBusConnection* UNUSED(connection), } zathura_rectangle_t* rect = g_try_malloc0(sizeof(zathura_rectangle_t)); - if (rect == NULL) { - continue; + if (rect == NULL || rectangles[temp_page] == NULL) { + g_variant_iter_free(secondary_iter); + for (unsigned int p = 0; p != number_of_pages; ++p) { + girara_list_free(rectangles[p]); + } + g_free(rectangles); + g_free(rect); + g_dbus_method_invocation_return_error(invocation, G_DBUS_ERROR, + G_DBUS_ERROR_NO_MEMORY, + "Failed to allocate memory."); + return; } + *rect = temp_rect; girara_list_append(rectangles[temp_page], rect); } diff --git a/plugin.c b/plugin.c index 3606d3d..b3e59cf 100644 --- a/plugin.c +++ b/plugin.c @@ -164,6 +164,9 @@ zathura_plugin_manager_load(zathura_plugin_manager_t* plugin_manager) plugin = g_try_malloc0(sizeof(zathura_plugin_t)); if (plugin == NULL) { + girara_error("Failed to allocate memory for plugin."); + g_free(path); + g_module_close(handle); continue; } @@ -301,7 +304,7 @@ plugin_mapping_new(zathura_plugin_manager_t* plugin_manager, const gchar* type, } GIRARA_LIST_FOREACH_END(plugin_manager->type_plugin_mapping, zathura_type_plugin_mapping_t*, iter, mapping); - zathura_type_plugin_mapping_t* mapping = g_try_malloc(sizeof(zathura_type_plugin_mapping_t)); + zathura_type_plugin_mapping_t* mapping = g_try_malloc0(sizeof(zathura_type_plugin_mapping_t)); if (mapping == NULL) { return false; } diff --git a/render.c b/render.c index ffea0da..a8a7dc7 100644 --- a/render.c +++ b/render.c @@ -122,14 +122,13 @@ page_cache_init(ZathuraRenderer* renderer, size_t cache_size) { private_t* priv = GET_PRIVATE(renderer); - priv->page_cache.size = cache_size; - priv->page_cache.cache = g_try_malloc(cache_size * sizeof(int)); + priv->page_cache.size = cache_size; + priv->page_cache.cache = g_try_malloc0(cache_size * sizeof(int)); if (priv->page_cache.cache == NULL) { return false; } page_cache_invalidate_all(renderer); - return true; } @@ -142,6 +141,7 @@ zathura_renderer_new(size_t cache_size) ZathuraRenderer* ret = ZATHURA_RENDERER(obj); if (page_cache_init(ret, cache_size) == false) { + g_object_unref(obj); return NULL; } @@ -646,7 +646,8 @@ render(render_job_t* job, ZathuraRenderRequest* request, ZathuraRenderer* render const double width = zathura_page_get_width(page); const double real_scale = page_calc_height_width(document, height, width, - &page_height, &page_width, false); + &page_height, &page_width, + false); cairo_surface_t* surface = cairo_image_surface_create(CAIRO_FORMAT_RGB24, @@ -685,7 +686,7 @@ render(render_job_t* job, ZathuraRenderRequest* request, ZathuraRenderer* render /* before recoloring, check if we've been aborted */ if (priv->about_to_close == true || job->aborted == true) { girara_debug("Rendering of page %d aborted", - zathura_page_get_index(request_priv->page) + 1); + zathura_page_get_index(request_priv->page) + 1); remove_job_and_free(job); cairo_surface_destroy(surface); return true; @@ -696,12 +697,13 @@ render(render_job_t* job, ZathuraRenderRequest* request, ZathuraRenderer* render recolor(priv, page_width, page_height, surface); } - emit_completed_signal_t* ecs = g_try_malloc(sizeof(emit_completed_signal_t)); + emit_completed_signal_t* ecs = g_try_malloc0(sizeof(emit_completed_signal_t)); if (ecs == NULL) { + cairo_surface_destroy(surface); return false; } - ecs->job = job; + ecs->job = job; ecs->surface = cairo_surface_reference(surface); /* emit signal from the main context, i.e. the main thread */ diff --git a/utils.c b/utils.c index cb55110..b9c00f9 100644 --- a/utils.c +++ b/utils.c @@ -211,8 +211,8 @@ replace_substring(const char* string, const char* old, const char* new) size_t new_len = strlen(new); /* count occurrences */ - unsigned int count = 0; - unsigned int i = 0; + size_t count = 0; + size_t i = 0; for (i = 0; string[i] != '\0'; i++) { if (strstr(&string[i], old) == &string[i]) { @@ -230,9 +230,8 @@ replace_substring(const char* string, const char* old, const char* new) return NULL; } - i = 0; - /* replace */ + i = 0; while (*string != '\0') { if (strstr(string, old) == string) { strncpy(&ret[i], new, new_len); diff --git a/zathura.c b/zathura.c index 821bae2..75aabc5 100644 --- a/zathura.c +++ b/zathura.c @@ -1251,14 +1251,14 @@ zathura_jumplist_append_jump(zathura_t* zathura) { g_return_if_fail(zathura != NULL && zathura->jumplist.list != NULL); - zathura_jump_t *jump = g_try_malloc(sizeof(zathura_jump_t)); + zathura_jump_t* jump = g_try_malloc0(sizeof(zathura_jump_t)); if (jump == NULL) { return; } jump->page = 0; - jump->x = 0.0; - jump->y = 0.0; + jump->x = 0.0; + jump->y = 0.0; girara_list_append(zathura->jumplist.list, jump); if (zathura->jumplist.size == 0) {