diff --git a/src/libs/libAmpr.cpp b/src/libs/libAmpr.cpp index dfdc17305..ebb32c402 100644 --- a/src/libs/libAmpr.cpp +++ b/src/libs/libAmpr.cpp @@ -52,6 +52,7 @@ struct ResolvedPathInfo { static std::mutex g_mutex; static uint32_t g_next_submission_id = 1; +static uint32_t g_next_file_id = 1; static std::unordered_map g_submissions; static std::condition_variable g_submission_cv; static std::unordered_map g_files; @@ -92,15 +93,6 @@ static bool WriteGuest(uint64_t addr, const T& value) { return WriteGuestBytes(addr, &value, sizeof(T)); } -static uint32_t ComputeFileId(const char* guest_path) { - uint32_t hash = 2166136261u; - for (auto* p = reinterpret_cast(guest_path); p != nullptr && *p != 0; ++p) { - hash ^= *p; - hash *= 16777619u; - } - return hash & static_cast(std::numeric_limits::max()); -} - static int ReadGuestCString(uint64_t addr, char* out, size_t out_size) { if (addr == 0) { return LibKernel::KERNEL_ERROR_EFAULT; @@ -120,18 +112,6 @@ static int ReadGuestCString(uint64_t addr, char* out, size_t out_size) { return LibKernel::KERNEL_ERROR_ENAMETOOLONG; } -static void RegisterHostPathLocked(uint32_t file_id, const std::string& host_path, - uint64_t file_size, bool is_dir) { - g_files[file_id] = host_path; - g_file_sizes[file_id] = is_dir ? 0 : file_size; -} - -static void RegisterHostPath(uint32_t file_id, const std::string& host_path, uint64_t file_size, - bool is_dir) { - std::scoped_lock lock(g_mutex); - RegisterHostPathLocked(file_id, host_path, file_size, is_dir); -} - static bool TryGetHostPath(uint32_t file_id, std::string* out) { std::scoped_lock lock(g_mutex); const auto it = g_files.find(file_id); @@ -213,7 +193,6 @@ static int ResolveOnePath(const char* guest_path, uint32_t* id, uint64_t* size) if (!found) { const auto real_path = LibKernel::FileSystem::GetRealFilename(path); - info.file_id = AprShared::ComputeFileId(guest_path); info.host_path = Common::PathToString(real_path); if (Common::File::IsDirectoryExisting(real_path)) { @@ -232,7 +211,14 @@ static int ResolveOnePath(const char* guest_path, uint32_t* id, uint64_t* size) if (!inserted) { info = it->second; } else if (info.result == OK) { - RegisterHostPathLocked(info.file_id, info.host_path, info.file_size, info.is_dir); + // Allocate once under the cache lock; distinct paths can share a hash. + if (g_next_file_id == 0xffffffffu) { + info.result = it->second.result = LibKernel::KERNEL_ERROR_ENFILE; + } else { + info.file_id = it->second.file_id = g_next_file_id++; + g_files.emplace(info.file_id, info.host_path); + g_file_sizes.emplace(info.file_id, info.is_dir ? 0 : info.file_size); + } } else if (info.result == LibKernel::KERNEL_ERROR_ENOENT) { log_missing = true; } @@ -240,8 +226,6 @@ static int ResolveOnePath(const char* guest_path, uint32_t* id, uint64_t* size) if (log_missing) { LOGF("\tAPR resolve missing path: %s -> %s\n", guest_path, info.host_path.c_str()); } - } else if (info.result == OK) { - AprShared::RegisterHostPath(info.file_id, info.host_path, info.file_size, info.is_dir); } if (info.result != OK) { diff --git a/tests/KernelFileSystemTests.cpp b/tests/KernelFileSystemTests.cpp index 73efc78f1..193483def 100644 --- a/tests/KernelFileSystemTests.cpp +++ b/tests/KernelFileSystemTests.cpp @@ -857,6 +857,36 @@ void CheckAprPaths(const std::filesystem::path &root) { ids[0] == 0xffffffffu && ids[1] == expected_id && sizes[0] == 0 && sizes[1] == 3, "APR foreach reports a missing path and continues to the valid file"); + // These paths share the old 31-bit FNV-1a ID (0x122d1544). + const char *collision_paths[] = {"perf-audit/test_00015ddf.bin", + "perf-audit/test_000389b8.bin"}; + Check(std::filesystem::create_directory(root / "perf-audit"), "create APR collision directory"); + for (size_t i = 0; i < 2; ++i) { + Check(fixture.Create(root / collision_paths[i]), "create APR collision fixture"); + fixture.Write(i == 0 ? "APR" : "OTHER", i == 0 ? 3 : 5); + fixture.Close(); + } + uint32_t collision_ids[2] = {}; + Check(resolve("/app0/", collision_paths, 2, collision_ids, sizes, &error_index) == OK && + collision_ids[0] != collision_ids[1] && collision_ids[0] != 0xffffffffu && + collision_ids[1] != 0xffffffffu && sizes[0] == 3 && sizes[1] == 5, + "APR assigns distinct valid IDs to colliding paths"); + const auto *stat_symbol = symbols.FindByNid("ApkYaHb8Sek", Loader::SymbolType::Func); + const auto *size_symbol = symbols.FindByNid("WvEu7yl3Ivg", Loader::SymbolType::Func); + Check(stat_symbol && size_symbol, "APR file metadata exports are registered"); + using Stat = int (KYTY_SYSV_ABI *)(uint32_t, FileSystem::FileStat *); + using Size = int (KYTY_SYSV_ABI *)(uint32_t, uint64_t *); + for (size_t i = 0; i < 2; ++i) { + FileSystem::FileStat stat {}; + uint64_t size = 0; + Check(resolve("/app0/", &collision_paths[i], 1, ids, nullptr, &error_index) == OK && + ids[0] == collision_ids[i] && + reinterpret_cast(stat_symbol->vaddr)(ids[0], &stat) == OK && + reinterpret_cast(size_symbol->vaddr)(ids[0], &size) == OK && + stat.st_size == static_cast(sizes[i]) && size == sizes[i], + "APR re-resolution preserves each file's ID, host path and cached size"); + } + // PATH_MAX includes NUL; all components remain below NAME_MAX (255). std::string longest = "/app0/"; for (int i = 0; i < 3; ++i) { @@ -880,7 +910,7 @@ void CheckAprPaths(const std::filesystem::path &root) { Check(resolve(unterminated.data(), paths, 1, ids, sizes, &error_index) == -1 && *Libs::Posix::GetErrorAddr() == Libs::Posix::POSIX_ENAMETOOLONG, "APR rejects an unterminated prefix"); - CheckAmprOrdering(symbols, expected_id); + CheckAmprOrdering(symbols, collision_ids[0]); FileSystem::Umount("/app0"); }