From d0b2ff986cbfde32e5b0378f1a2f217fc8a695ca Mon Sep 17 00:00:00 2001 From: zhang hongyuan <100520587+Metastarx@users.noreply.github.com> Date: Fri, 9 Oct 2026 01:03:09 +0800 Subject: [PATCH 1/2] fix(linglong): stop migration clobbering refs and mis-ordering versions The already-migrated check built its lookup key with std::string::append on a single shared buffer. append mutates its receiver and returns it, so the repository prefix accumulated one ref per loop iteration and every lookup after the first used a key that can never exist. Such a ref was migrated a second time, and ostree_repo_transaction_set_ref pointed the prefixed ref at the stale legacy commit, overwriting the commit and the layer symlink that were already there. Version::operator< also evaluated minor and patch even when a more significant component already differed, so 2.0.0 compared less than 1.7.0. The 1.7.0 gate in dispatchMigrations relies on this being a real ordering, otherwise a repository newer than 1.7.0 could re-run the legacy migration. Signed-off-by: zhang hongyuan <100520587+Metastarx@users.noreply.github.com> --- libs/linglong/src/linglong/repo/migrate.cpp | 33 ++- .../src/linglong/repo/migrate_test.cpp | 210 ++++++++++++++++++ 2 files changed, 240 insertions(+), 3 deletions(-) diff --git a/libs/linglong/src/linglong/repo/migrate.cpp b/libs/linglong/src/linglong/repo/migrate.cpp index a49230657..28eea6c72 100644 --- a/libs/linglong/src/linglong/repo/migrate.cpp +++ b/libs/linglong/src/linglong/repo/migrate.cpp @@ -30,9 +30,23 @@ struct Version int minor{ 0 }; int patch{ 0 }; + // Compare (major, minor, patch) lexicographically. The previous + // `lhs.major < rhs.major || lhs.minor < rhs.minor || lhs.patch < rhs.patch` + // chain is not a valid ordering: it also evaluates the minor/patch + // components when the more significant ones differ, so e.g. 2.0.0 compared + // less than 1.7.0. The 1.7.0 gate in dispatchMigrations relies on this + // being a real ordering; otherwise a repository whose .version is newer + // than 1.7.0 (but has a smaller minor or patch) would incorrectly re-run + // the legacy ref migration. friend bool operator<(const Version &lhs, const Version &rhs) noexcept { - return lhs.major < rhs.major || lhs.minor < rhs.minor || lhs.patch < rhs.patch; + if (lhs.major != rhs.major) { + return lhs.major < rhs.major; + } + if (lhs.minor != rhs.minor) { + return lhs.minor < rhs.minor; + } + return lhs.patch < rhs.patch; } }; @@ -85,7 +99,7 @@ int migrateRef(OstreeRepo *repo, const MigrateRefData &data) } std::unordered_map needMigrate; - auto refPrefix = data.repoName + ":"; + const std::string refPrefix = data.repoName + ":"; for (auto it = allRefs.begin(); it != allRefs.end();) { if (it->first.rfind(refPrefix, 0) == 0) { ++it; @@ -95,8 +109,21 @@ int migrateRef(OstreeRepo *repo, const MigrateRefData &data) } } + // Drop every legacy ref whose prefixed counterpart already exists in the + // repository. Re-migrating such a ref would call + // ostree_repo_transaction_set_ref(repo, repoName, ref, legacyChecksum) and + // point the existing prefixed ref at the stale legacy commit, overwriting + // both the commit and the layer symlink that were already there. Every ref + // that still needs migration must therefore be checked against its real + // prefixed name before the transaction is prepared; the prefixed name is + // the repository name, a colon and the legacy ref. + // + // Build the lookup key from scratch on every iteration. std::string::append + // mutates its receiver and returns a reference to it, so reusing a single + // growing buffer makes the prefix accumulate one ref per iteration and + // turns every lookup after the first into a key that can never exist. for (auto it = needMigrate.begin(); it != needMigrate.end();) { - auto tmpRef = refPrefix.append(it->first); + const std::string tmpRef = refPrefix + std::string{ it->first }; if (allRefs.find(tmpRef) != allRefs.end()) { it = needMigrate.erase(it); } else { diff --git a/libs/linglong/tests/ll-tests/src/linglong/repo/migrate_test.cpp b/libs/linglong/tests/ll-tests/src/linglong/repo/migrate_test.cpp index cd58472fe..39e3915c8 100644 --- a/libs/linglong/tests/ll-tests/src/linglong/repo/migrate_test.cpp +++ b/libs/linglong/tests/ll-tests/src/linglong/repo/migrate_test.cpp @@ -16,6 +16,9 @@ #include #include +#include +#include +#include using namespace linglong::repo; using namespace linglong::api::types::v1; @@ -31,6 +34,100 @@ RepoConfigV2 makeConfig() }; } +// A single ref to seed into a test ostree repository. A null prefix selects +// the legacy, unprefixed ref namespace written by repositories created before +// the 1.7.0 migration. +struct RefEntry +{ + const char *prefix; + const char *ref; + const char *checksum; +}; + +// Create a bare ostree repository at repoPath and return an owned handle. The +// caller stores the result in a g_autoptr so the handle is released when the +// test ends. +OstreeRepo *createBareRepo(const std::filesystem::path &repoPath, GError **err) +{ + g_autoptr(GFile) gf = g_file_new_for_path(repoPath.c_str()); + OstreeRepo *repo = ostree_repo_new(gf); + if (repo == nullptr) { + return nullptr; + } + + if (ostree_repo_create(repo, OSTREE_REPO_MODE_BARE, nullptr, err) == FALSE) { + g_object_unref(repo); + return nullptr; + } + + return repo; +} + +// Commit a batch of refs atomically so the on-disk ref table holds both the +// legacy names and the already-prefixed names before the migration runs. +bool writeRefs(OstreeRepo *repo, const std::vector &entries, GError **err) +{ + if (ostree_repo_prepare_transaction(repo, nullptr, nullptr, err) == FALSE) { + return false; + } + + for (const auto &entry : entries) { + ostree_repo_transaction_set_ref(repo, entry.prefix, entry.ref, entry.checksum); + } + + return ostree_repo_commit_transaction(repo, nullptr, nullptr, err) != 0; +} + +// Read the whole ref table into a name to commit map so tests can assert on the +// exact commit a ref points at, not merely on the ref being present. +std::map readRefs(OstreeRepo *repo, GError **err) +{ + g_autoptr(GHashTable) refs = nullptr; + std::map result; + if (ostree_repo_list_refs(repo, nullptr, &refs, nullptr, err) == FALSE) { + return result; + } + + g_hash_table_foreach( + refs, + [](gpointer key, gpointer value, gpointer data) { + auto &out = *static_cast *>(data); + out.emplace(static_cast(key), static_cast(value)); + }, + &result); + + return result; +} + +// Create the legacy /layers/ path so the migration can create the +// ref to checksum symlink exactly as it would on a real installation. +void createLegacyLayer(const std::filesystem::path &root, const std::string &ref) +{ + auto layerPath = root / "layers" / ref; + std::filesystem::create_directories(layerPath.parent_path()); + std::ofstream{ layerPath } << ""; +} + +// Distinct, syntactically valid 64-character hex commit ids. The migration +// copies ref values without validating the commit, but every ref in a test +// uses its own id so an overwritten ref is detectable. +constexpr const char *kChecksumLegacyA = + "1111111111111111111111111111111111111111111111111111111111111111"; +constexpr const char *kChecksumLegacyB = + "2222222222222222222222222222222222222222222222222222222222222222"; +constexpr const char *kChecksumLegacyC = + "3333333333333333333333333333333333333333333333333333333333333333"; +constexpr const char *kChecksumKeptA = + "4444444444444444444444444444444444444444444444444444444444444444"; +constexpr const char *kChecksumKeptB = + "5555555555555555555555555555555555555555555555555555555555555555"; +constexpr const char *kChecksumKeptC = + "6666666666666666666666666666666666666666666666666666666666666666"; + +constexpr const char *kLegacyRefA = "com.example.a/1.0.0/x86_64/main"; +constexpr const char *kLegacyRefB = "com.example.b/1.0.0/x86_64/main"; +constexpr const char *kLegacyRefC = "com.example.c/1.0.0/x86_64/main"; + TEST(MigrateTest, NonExistentRootIsNoChange) { TempDir dir; @@ -191,4 +288,117 @@ TEST(MigrateTest, RealOstreeRepoMigratesUnprefixedRefs) EXPECT_TRUE(std::filesystem::is_symlink(link)); } +TEST(MigrateTest, LegacyRefsWithExistingPrefixedCounterpartsAreNotMigrated) +{ + TempDir dir; + std::ofstream{ dir.path() / ".version" } << "1.5.0"; + + g_autoptr(GError) gErr = nullptr; + g_autoptr(OstreeRepo) repo = createBareRepo(dir.path() / "repo", &gErr); + ASSERT_NE(repo, nullptr) << (gErr ? gErr->message : "ostree_repo_create failed"); + + // Every legacy unprefixed ref already has a prefixed counterpart holding a + // different commit. The migration must notice this and leave all of them + // alone. The previous growing-prefix lookup re-migrated all but the first + // ref and overwrote the commits that were already present. + ASSERT_TRUE(writeRefs(repo, + { + { nullptr, kLegacyRefA, kChecksumLegacyA }, + { nullptr, kLegacyRefB, kChecksumLegacyB }, + { nullptr, kLegacyRefC, kChecksumLegacyC }, + { "stable", kLegacyRefA, kChecksumKeptA }, + { "stable", kLegacyRefB, kChecksumKeptB }, + { "stable", kLegacyRefC, kChecksumKeptC }, + }, + &gErr)) + << (gErr ? gErr->message : "writing refs failed"); + + // No prefixed counterpart is missing, so migrateRef reports no change and + // must not prepare a transaction at all. + EXPECT_EQ(tryMigrate(dir.path(), makeConfig()), MigrateResult::NoChange); + + auto refs = readRefs(repo, &gErr); + ASSERT_FALSE(refs.empty()) << (gErr ? gErr->message : "listing refs failed"); + + // The already-migrated refs must still point at their original commits. + EXPECT_EQ(refs["stable:com.example.a/1.0.0/x86_64/main"], kChecksumKeptA); + EXPECT_EQ(refs["stable:com.example.b/1.0.0/x86_64/main"], kChecksumKeptB); + EXPECT_EQ(refs["stable:com.example.c/1.0.0/x86_64/main"], kChecksumKeptC); + + // The legacy refs themselves are never rewritten by this step. + EXPECT_EQ(refs["com.example.a/1.0.0/x86_64/main"], kChecksumLegacyA); + EXPECT_EQ(refs["com.example.b/1.0.0/x86_64/main"], kChecksumLegacyB); + EXPECT_EQ(refs["com.example.c/1.0.0/x86_64/main"], kChecksumLegacyC); +} + +TEST(MigrateTest, MigrationAddsOnlyMissingPrefixedRefs) +{ + TempDir dir; + std::ofstream{ dir.path() / ".version" } << "1.5.0"; + + g_autoptr(GError) gErr = nullptr; + g_autoptr(OstreeRepo) repo = createBareRepo(dir.path() / "repo", &gErr); + ASSERT_NE(repo, nullptr) << (gErr ? gErr->message : "ostree_repo_create failed"); + + // Three legacy refs, but only B already has its prefixed counterpart. The + // migration must create prefixed A and C from the legacy commits and must + // keep the commit that stable:B already points at. + ASSERT_TRUE(writeRefs(repo, + { + { nullptr, kLegacyRefA, kChecksumLegacyA }, + { nullptr, kLegacyRefB, kChecksumLegacyB }, + { nullptr, kLegacyRefC, kChecksumLegacyC }, + { "stable", kLegacyRefB, kChecksumKeptB }, + }, + &gErr)) + << (gErr ? gErr->message : "writing refs failed"); + + createLegacyLayer(dir.path(), kLegacyRefA); + createLegacyLayer(dir.path(), kLegacyRefC); + + EXPECT_EQ(tryMigrate(dir.path(), makeConfig()), MigrateResult::Success); + + auto refs = readRefs(repo, &gErr); + ASSERT_FALSE(refs.empty()) << (gErr ? gErr->message : "listing refs failed"); + + EXPECT_EQ(refs["stable:com.example.a/1.0.0/x86_64/main"], kChecksumLegacyA); + EXPECT_EQ(refs["stable:com.example.b/1.0.0/x86_64/main"], kChecksumKeptB); + EXPECT_EQ(refs["stable:com.example.c/1.0.0/x86_64/main"], kChecksumLegacyC); + + // The migrated refs get a layers/ symlink pointing at the legacy + // layer directory. + EXPECT_TRUE(std::filesystem::is_symlink(dir.path() / "layers" / kChecksumLegacyA)); + EXPECT_TRUE(std::filesystem::is_symlink(dir.path() / "layers" / kChecksumLegacyC)); + // The pre-existing prefixed ref must not produce a symlink of its own. + EXPECT_FALSE(std::filesystem::exists(dir.path() / "layers" / kChecksumKeptB)); +} + +TEST(MigrateTest, VersionNewerThan170SkipsLegacyRefMigration) +{ + TempDir dir; + // 2.0.0 has a larger major but smaller minor and patch than 1.7.0. The old + // non-lexicographic comparison ordered it below 1.7.0 and re-ran the + // migration; a real ordering must skip it. + std::ofstream{ dir.path() / ".version" } << "2.0.0"; + + g_autoptr(GError) gErr = nullptr; + g_autoptr(OstreeRepo) repo = createBareRepo(dir.path() / "repo", &gErr); + ASSERT_NE(repo, nullptr) << (gErr ? gErr->message : "ostree_repo_create failed"); + + ASSERT_TRUE(writeRefs(repo, { { nullptr, kLegacyRefA, kChecksumLegacyA } }, &gErr)) + << (gErr ? gErr->message : "writing refs failed"); + + createLegacyLayer(dir.path(), kLegacyRefA); + + EXPECT_EQ(tryMigrate(dir.path(), makeConfig()), MigrateResult::Success); + + auto refs = readRefs(repo, &gErr); + ASSERT_FALSE(refs.empty()) << (gErr ? gErr->message : "listing refs failed"); + + // The prefixed ref was not created and the legacy ref keeps its commit. + EXPECT_TRUE(refs.find("stable:com.example.a/1.0.0/x86_64/main") == refs.end()); + EXPECT_EQ(refs["com.example.a/1.0.0/x86_64/main"], kChecksumLegacyA); + EXPECT_FALSE(std::filesystem::exists(dir.path() / "layers" / kChecksumLegacyA)); +} + } // namespace From bb5d9253d043263be25b5cb2f40d150b22086124 Mon Sep 17 00:00:00 2001 From: zhang hongyuan <100520587+Metastarx@users.noreply.github.com> Date: Fri, 9 Oct 2026 22:03:35 +0800 Subject: [PATCH 2/2] fix(linglong): update the SPDX copyright year of migrate.cpp The license check requires the year range to span the last modification, so a file created in 2024 and touched in 2026 has to read 2024-2026. This is the only file the check flagged on this branch; the new test file already carries 2026. Signed-off-by: zhang hongyuan <100520587+Metastarx@users.noreply.github.com> --- libs/linglong/src/linglong/repo/migrate.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/linglong/src/linglong/repo/migrate.cpp b/libs/linglong/src/linglong/repo/migrate.cpp index 28eea6c72..b7553674a 100644 --- a/libs/linglong/src/linglong/repo/migrate.cpp +++ b/libs/linglong/src/linglong/repo/migrate.cpp @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: 2024 UnionTech Software Technology Co., Ltd. +// SPDX-FileCopyrightText: 2024-2026 UnionTech Software Technology Co., Ltd. // // SPDX-License-Identifier: LGPL-3.0-or-later