From 9736bece376041c0ad712102681ab93a24de07c7 Mon Sep 17 00:00:00 2001 From: Omari Stephens Date: Mon, 1 Jul 2024 21:42:10 +0000 Subject: [PATCH] Fixes a double-ref-counting bug in `file_data_new_simple` and `file_data_new_group` The bugs would cause all FileData objects created by `file_data_new_simple` or `file_data_new_group` to start off with a ref-count of 2, even though the only one reference to it had been created (the one returned by each method). As a result, creating the FileData and then immediately calling `file_data_unref` would not free it as would be expected. Also zero-initializes `struct stat st` so that its contents will be valid even if `stat_utf8` fails. --- src/filedata/filedata.cc | 30 +++++++++++++++--------------- src/tests/filedata/filedata.cc | 2 -- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/filedata/filedata.cc b/src/filedata/filedata.cc index 9cfa169fe..18ce6567b 100644 --- a/src/filedata/filedata.cc +++ b/src/filedata/filedata.cc @@ -496,7 +496,7 @@ FileData *FileData::file_data_new_local(const gchar *path, struct stat *st, gboo FileData *FileData::file_data_new_simple(const gchar *path_utf8, FileDataContext *context) { - struct stat st; + struct stat st{}; if (!stat_utf8(path_utf8, &st)) { @@ -510,11 +510,11 @@ FileData *FileData::file_data_new_simple(const gchar *path_utf8, FileDataContext } auto *fd = static_cast(g_hash_table_lookup(context->file_data_pool, path_utf8)); - // TODO(xsdg): This looks like it double-refs fd if it needed to be - // created. Figure out what should happen here and either fix or - // document. - if (!fd) fd = file_data_new(path_utf8, &st, TRUE, context); - if (fd) + if (!fd) + { + fd = file_data_new(path_utf8, &st, TRUE, context); + } + else { ::file_data_ref(fd); } @@ -1211,16 +1211,12 @@ void FileData::file_data_basename_hash_to_sidecars(gpointer, gpointer value, gpo FileData *FileData::file_data_new_group(const gchar *path_utf8, FileDataContext *context) { - gchar *dir; - struct stat st; - FileData *fd; - GList *files; - if (context == nullptr) { context = FileData::DefaultFileDataContext(); } + struct stat st{}; if (!stat_utf8(path_utf8, &st)) { st.st_size = 0; @@ -1230,13 +1226,17 @@ FileData *FileData::file_data_new_group(const gchar *path_utf8, FileDataContext if (S_ISDIR(st.st_mode)) return file_data_new(path_utf8, &st, TRUE, context); - dir = remove_level_from_path(path_utf8); + gchar *dir = remove_level_from_path(path_utf8); + GList *files; FileList::read_list_real(dir, &files, nullptr, TRUE); - fd = static_cast(g_hash_table_lookup(context->file_data_pool, path_utf8)); - if (!fd) fd = file_data_new(path_utf8, &st, TRUE, context); - if (fd) + auto *fd = static_cast(g_hash_table_lookup(context->file_data_pool, path_utf8)); + if (!fd) + { + fd = file_data_new(path_utf8, &st, TRUE, context); + } + else { ::file_data_ref(fd); } diff --git a/src/tests/filedata/filedata.cc b/src/tests/filedata/filedata.cc index 97bd993ef..f243f6f1e 100644 --- a/src/tests/filedata/filedata.cc +++ b/src/tests/filedata/filedata.cc @@ -124,7 +124,6 @@ TEST_F(FileDataTest, FileDataNewSimpleAndFree) auto *_fd = FileData::file_data_new_simple("/does/not/exist.jpg", &context); ASSERT_NE(_fd, nullptr); ASSERT_EQ(1, context.global_file_data_count); - // This currently fails because of a bug in file_data_new_simple. ASSERT_EQ(1, _fd->ref); _fd->file_data_unref(); @@ -139,7 +138,6 @@ TEST_F(FileDataTest, FileDataNewGroupAndFree) auto *_fd = FileData::file_data_new_group("/does/not/exist/file.jpg", &context); ASSERT_NE(_fd, nullptr); EXPECT_EQ(1, context.global_file_data_count); - // This currently fails because of a bug in file_data_new_simple. ASSERT_EQ(1, _fd->ref); _fd->file_data_unref();