已合并
Description:fix clean code #4370
EurusHomles-zH创建于 7月20日
Description:fix clean code #4370
已合并
共 7 个文件变更+90-24
| @@ -111,6 +111,8 @@ CollectResult<int32_t> HiViewServiceMemoryDelegate::RequestUiTree(int32_t pid, | |||
| 111 | ret.retCode = UCollect::UcError::SUCCESS; | 111 | ret.retCode = UCollect::UcError::SUCCESS; |
| 112 | } else { | 112 | } else { |
| 113 | ret.retCode = UCollect::UcError::SYSTEM_ERROR; | 113 | ret.retCode = UCollect::UcError::SYSTEM_ERROR; |
| 114 | + // Store the specific error code for caller to diagnose the failure reason | ||
| 115 | + ret.data = proxyRet; | ||
| 114 | } | 116 | } |
| 115 | return ret; | 117 | return ret; |
| 116 | } | 118 | } |
| @@ -20,6 +20,7 @@ | |||
| 20 | 20 | ||
| 21 | 21 | ||
| 22 | 22 | ||
| 23 | + | ||
| 23 | 24 | ||
| 24 | namespace OHOS { | 25 | namespace OHOS { |
| 25 | namespace HiviewDFX { | 26 | namespace HiviewDFX { |
| @@ -151,6 +152,10 @@ void ConvertLogFileName(const std::string& oldFileName, std::string& newFileName | |||
| 151 | bool ShouldRefinedLogFileName(int32_t uid, const std::string& pathHolder) | 152 | bool ShouldRefinedLogFileName(int32_t uid, const std::string& pathHolder) |
| 152 | { | 153 | { |
| 153 | std::string eventConfigDir = BundleUtil::GetSandBoxPath(uid, "base", pathHolder, "cache/eventConfig"); | 154 | std::string eventConfigDir = BundleUtil::GetSandBoxPath(uid, "base", pathHolder, "cache/eventConfig"); |
| 155 | + if (StringUtil::ContainsPathTraversal(eventConfigDir)) { | ||
| 156 | + HIVIEW_LOGE("pathHolder contains path traversal sequence: %{public}s", pathHolder.c_str()); | ||
| 157 | + return false; | ||
| 158 | + } | ||
| 154 | if (eventConfigDir.empty()) { | 159 | if (eventConfigDir.empty()) { |
| 155 | HIVIEW_LOGE("Current sandbox eventConfig path is not exist."); | 160 | HIVIEW_LOGE("Current sandbox eventConfig path is not exist."); |
| 156 | return false; | 161 | return false; |
| @@ -183,8 +188,6 @@ void RefineLogFilePaths(Json::Value& eventJson, const std::string& oldLogPath, s | |||
| 183 | } | 188 | } |
| 184 | std::string oldFileName = FileUtil::ExtractFileName(oldLogPath); | 189 | std::string oldFileName = FileUtil::ExtractFileName(oldLogPath); |
| 185 | ConvertLogFileName(oldFileName, newFileName, pid, resourceType); | 190 | ConvertLogFileName(oldFileName, newFileName, pid, resourceType); |
| 186 | - HIVIEW_LOGI("use refined file name: %{public}s, curLogName: %{public}s", oldLogPath.c_str(), | ||
| 187 | - newFileName.c_str()); | ||
| 188 | } | 191 | } |
| 189 | 192 | ||
| 190 | } // namespace HiviewDFX | 193 | } // namespace HiviewDFX |
| @@ -341,6 +341,7 @@ | |||
| 341 | "OHOS::HiviewDFX::FileUtil::LoadLinesFromFile(std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&, std::__h::vector<std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>>, std::__h::allocator<std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>>>>&)"; | 341 | "OHOS::HiviewDFX::FileUtil::LoadLinesFromFile(std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&, std::__h::vector<std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>>, std::__h::allocator<std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>>>>&)"; |
| 342 | "OHOS::HiviewDFX::StringUtil::EscapeJsonStringValue(std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&)"; | 342 | "OHOS::HiviewDFX::StringUtil::EscapeJsonStringValue(std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&)"; |
| 343 | "OHOS::HiviewDFX::StringUtil::UnescapeJsonStringValue(std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&)"; | 343 | "OHOS::HiviewDFX::StringUtil::UnescapeJsonStringValue(std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&)"; |
| 344 | + "OHOS::HiviewDFX::StringUtil::ContainsPathTraversal(std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&)"; | ||
| 344 | "OHOS::HiviewDFX::FileUtil::SaveStringToFd(int, std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&)"; | 345 | "OHOS::HiviewDFX::FileUtil::SaveStringToFd(int, std::__h::basic_string<char, std::__h::char_traits<char>, std::__h::allocator<char>> const&)"; |
| 345 | "OHOS::HiviewDFX::TimeUtil::GetTimeZone()"; | 346 | "OHOS::HiviewDFX::TimeUtil::GetTimeZone()"; |
| 346 | "OHOS::HiviewDFX::FreezeJsonUtil::GetFilePath(long, long, unsigned long long)"; | 347 | "OHOS::HiviewDFX::FreezeJsonUtil::GetFilePath(long, long, unsigned long long)"; |
| @@ -152,6 +152,11 @@ bool StartWith(const std::string& str, const std::string& sub); | |||
| 152 | bool EndWith(const std::string& str, const std::string& sub); | 152 | bool EndWith(const std::string& str, const std::string& sub); |
| 153 | 153 | ||
| 154 | bool IsValidRegex(const std::string& regStr); | 154 | bool IsValidRegex(const std::string& regStr); |
| 155 | + | ||
| 156 | +/** | ||
| 157 | + * Check if the path contains path traversal sequences | ||
| 158 | + */ | ||
| 159 | +bool ContainsPathTraversal(const std::string& path); | ||
| 155 | } // namespace StringUtil | 160 | } // namespace StringUtil |
| 156 | } // namespace HiviewDFX | 161 | } // namespace HiviewDFX |
| 157 | } // namespace OHOS | 162 | } // namespace OHOS |
| @@ -434,6 +434,18 @@ bool IsValidRegex(const std::string& regStr) | |||
| 434 | regfree(®); | 434 | regfree(®); |
| 435 | return (status == REG_OK); | 435 | return (status == REG_OK); |
| 436 | } | 436 | } |
| 437 | + | ||
| 438 | +bool ContainsPathTraversal(const std::string& path) | ||
| 439 | +{ | ||
| 440 | + if (path.empty()) { | ||
| 441 | + return false; | ||
| 442 | + } | ||
| 443 | + constexpr size_t doubleDotLength = 2; | ||
| 444 | + return path.find("../") != std::string::npos || | ||
| 445 | + path.find("..\\") != std::string::npos || | ||
| 446 | + path.find("..") == 0 || | ||
| 447 | + path.rfind("..") == path.length() - doubleDotLength; | ||
| 448 | +} | ||
| 437 | } // namespace StringUtil | 449 | } // namespace StringUtil |
| 438 | } // namespace HiviewDFX | 450 | } // namespace HiviewDFX |
| 439 | } // namespace OHOS | 451 | } // namespace OHOS |
| @@ -86,6 +86,10 @@ bool FaultLogSanitizer::ConvertPathFromOriginLine(const std::string& line, std:: | |||
| 86 | 86 | ||
| 87 | // pathPrefix mean full sandbox path | 87 | // pathPrefix mean full sandbox path |
| 88 | if (pathPrefix.find(APP_SANDBOX_PREFIX) != std::string::npos) { | 88 | if (pathPrefix.find(APP_SANDBOX_PREFIX) != std::string::npos) { |
| 89 | + if (StringUtil::ContainsPathTraversal(bundleName)) { | ||
| 90 | + HIVIEW_LOGE("bundleName contains path traversal sequence: %{public}s", bundleName.c_str()); | ||
| 91 | + return false; | ||
| 92 | + } | ||
| 89 | pathPrefix = CONVERT_APP_SANBOPX_PREFIX + bundleName + "/" + fileName; | 93 | pathPrefix = CONVERT_APP_SANBOPX_PREFIX + bundleName + "/" + fileName; |
| 90 | } else { | 94 | } else { |
| 91 | pathPrefix = fullPath; | 95 | pathPrefix = fullPath; |
| @@ -203,7 +207,8 @@ std::string FaultLogSanitizer::ProcessArkTsLine(const std::string& line, const s | |||
| 203 | std::uintptr_t arkExtractorPtr = 0; | 207 | std::uintptr_t arkExtractorPtr = 0; |
| 204 | int ret = 0; | 208 | int ret = 0; |
| 205 | ret = DfxArk::Instance().ArkCreateJsSymbolExtractor(&arkExtractorPtr); | 209 | ret = DfxArk::Instance().ArkCreateJsSymbolExtractor(&arkExtractorPtr); |
| 206 | - if (ret < 0) { | 210 | + if (ret < 0 || arkExtractorPtr == 0) { |
| 211 | + HIVIEW_LOGE("Failed to create Ark JS symbol extractor, ret: %{public}d", ret); | ||
| 207 | return line; | 212 | return line; |
| 208 | } | 213 | } |
| 209 | JsFunction jsFunc; | 214 | JsFunction jsFunc; |
| @@ -282,35 +287,41 @@ std::vector<MapInfo> FaultLogSanitizer::LoadMaps(std::ifstream& file) | |||
| 282 | return maps; | 287 | return maps; |
| 283 | } | 288 | } |
| 284 | 289 | ||
| 285 | -bool FaultLogSanitizer::ParserArkTsStackInfo(const std::string& moduleName, const std::string& path) | 290 | +bool FaultLogSanitizer::OpenTempFile(const std::string& tempPath, FILE*& fp, int& tempFileFd) |
| 286 | { | 291 | { |
| 287 | - auto fileSize = FileUtil::GetFileSize(path); | 292 | + // If open is used, it needs to be used with fdsAN_CLOSE_WITH_TAG. Changed to fopen |
| 288 | - if (fileSize > ARKTS_STACK_MAX_FILE_SIZE) { | 293 | + fp = fopen(tempPath.c_str(), "w"); |
| 289 | - HIVIEW_LOGE("File size exceeds limit, path: %{public}s, size: %{public}" PRIu64, | ||
| 290 | - path.c_str(), fileSize); | ||
| 291 | - return false; | ||
| 292 | - } | ||
| 293 | - std::ifstream srcLogFile(path); | ||
| 294 | - if (!srcLogFile.is_open()) { | ||
| 295 | - HIVIEW_LOGE("Failed to open src file: %{public}s", path.c_str()); | ||
| 296 | - return false; | ||
| 297 | - } | ||
| 298 | - std::string tempPath = path + ".tmp"; | ||
| 299 | - FILE* fp = fopen(tempPath.c_str(), "w"); | ||
| 300 | if (fp == nullptr) { | 294 | if (fp == nullptr) { |
| 301 | HIVIEW_LOGE("Failed to open temp file: %{public}s", tempPath.c_str()); | 295 | HIVIEW_LOGE("Failed to open temp file: %{public}s", tempPath.c_str()); |
| 302 | - srcLogFile.close(); | ||
| 303 | return false; | 296 | return false; |
| 304 | } | 297 | } |
| 305 | - chmod(tempPath.c_str(), DEFAULT_LOG_FILE_MODE); | 298 | + // Restrict fopen-created file permissions to 0644 |
| 299 | + if (chmod(tempPath.c_str(), DEFAULT_LOG_FILE_MODE) != 0) { | ||
| 300 | + HIVIEW_LOGE("Failed to chmod temp file: %{public}s, err: %{public}s", tempPath.c_str(), strerror(errno)); | ||
| 301 | + (void)fclose(fp); | ||
| 302 | + return false; | ||
| 303 | + } | ||
| 304 | + tempFileFd = fileno(fp); | ||
| 305 | + if (tempFileFd < 0) { | ||
| 306 | + HIVIEW_LOGE("Failed to get file descriptor from FILE pointer, err: %{public}s", strerror(errno)); | ||
| 307 | + (void)fclose(fp); | ||
| 308 | + return false; | ||
| 309 | + } | ||
| 310 | + return true; | ||
| 311 | +} | ||
| 312 | + | ||
| 313 | +bool FaultLogSanitizer::WriteStackInfo(const std::string& moduleName, const std::string& path, | ||
| 314 | + const std::string& tempPath, std::ifstream& srcLogFile, | ||
| 315 | + int tempFileFd) | ||
| 316 | +{ | ||
| 317 | + // First traversal to read maps intervals, find address intervals corresponding to .hap and other formats | ||
| 306 | std::vector<MapInfo> maps = LoadMaps(srcLogFile); | 318 | std::vector<MapInfo> maps = LoadMaps(srcLogFile); |
| 307 | 319 | ||
| 308 | - int tempFileFd = fileno(fp); | ||
| 309 | std::string line; | 320 | std::string line; |
| 310 | - | ||
| 311 | srcLogFile.clear(); | 321 | srcLogFile.clear(); |
| 312 | srcLogFile.seekg(0, std::ios::beg); | 322 | srcLogFile.seekg(0, std::ios::beg); |
| 313 | 323 | ||
| 324 | + // Second traversal is to read .hap and other stacks from stack frames, and directly replace them after parsing | ||
| 314 | while (std::getline(srcLogFile, line)) { | 325 | while (std::getline(srcLogFile, line)) { |
| 315 | if (srcLogFile.eof()) { | 326 | if (srcLogFile.eof()) { |
| 316 | break; | 327 | break; |
| @@ -325,10 +336,39 @@ bool FaultLogSanitizer::ParserArkTsStackInfo(const std::string& moduleName, cons | |||
| 325 | FileUtil::SaveStringToFd(tempFileFd, "\n"); | 336 | FileUtil::SaveStringToFd(tempFileFd, "\n"); |
| 326 | } | 337 | } |
| 327 | srcLogFile.close(); | 338 | srcLogFile.close(); |
| 328 | - fsync(tempFileFd); | 339 | + if (fsync(tempFileFd) != 0) { |
| 329 | - (void)fclose(fp); | 340 | + HIVIEW_LOGE("Failed to sync temp file: %{public}s, err: %{public}s", tempPath.c_str(), strerror(errno)); |
| 330 | - fp = nullptr; | 341 | + return false; |
| 342 | + } | ||
| 343 | + return true; | ||
| 344 | +} | ||
| 331 | 345 | ||
| 346 | +bool FaultLogSanitizer::ParserArkTsStackInfo(const std::string& moduleName, const std::string& path) | ||
| 347 | +{ | ||
| 348 | + // If the file size is too large, stack unwinding during faultlog processing may encounter issues | ||
| 349 | + auto fileSize = FileUtil::GetFileSize(path); | ||
| 350 | + if (fileSize > ARKTS_STACK_MAX_FILE_SIZE) { | ||
| 351 | + HIVIEW_LOGE("File size exceeds limit, path: %{public}s, size: %{public}" PRIu64, | ||
| 352 | + path.c_str(), fileSize); | ||
| 353 | + return false; | ||
| 354 | + } | ||
| 355 | + std::ifstream srcLogFile(path); | ||
| 356 | + if (!srcLogFile.is_open()) { | ||
| 357 | + HIVIEW_LOGE("Failed to open src file: %{public}s", path.c_str()); | ||
| 358 | + return false; | ||
| 359 | + } | ||
| 360 | + std::string tempPath = path + ".tmp"; | ||
| 361 | + FILE* fp = nullptr; | ||
| 362 | + int tempFileFd = -1; | ||
| 363 | + if (!OpenTempFile(tempPath, fp, tempFileFd)) { | ||
| 364 | + srcLogFile.close(); | ||
| 365 | + return false; | ||
| 366 | + } | ||
| 367 | + if (!WriteStackInfo(moduleName, path, tempPath, srcLogFile, tempFileFd)) { | ||
| 368 | + (void)fclose(fp); | ||
| 369 | + return false; | ||
| 370 | + } | ||
| 371 | + (void)fclose(fp); | ||
| 332 | return FileUtil::RenameFile(tempPath.c_str(), path.c_str()); | 372 | return FileUtil::RenameFile(tempPath.c_str(), path.c_str()); |
| 333 | } | 373 | } |
| 334 | 374 | ||
| @@ -44,6 +44,9 @@ private: | |||
| 44 | std::string ProcessArkTsLine(const std::string& line, const std::string& packageName, | 44 | std::string ProcessArkTsLine(const std::string& line, const std::string& packageName, |
| 45 | const std::vector<MapInfo>& maps); | 45 | const std::vector<MapInfo>& maps); |
| 46 | std::vector<MapInfo> LoadMaps(std::ifstream& file); | 46 | std::vector<MapInfo> LoadMaps(std::ifstream& file); |
| 47 | + bool OpenTempFile(const std::string& tempPath, FILE*& fp, int& tempFileFd); | ||
| 48 | + bool WriteStackInfo(const std::string& moduleName, const std::string& path, | ||
| 49 | + const std::string& tempPath, std::ifstream& srcLogFile, int tempFileFd); | ||
| 47 | bool ParserArkTsStackInfo(const std::string& moduleName, const std::string& path); | 50 | bool ParserArkTsStackInfo(const std::string& moduleName, const std::string& path); |
| 48 | bool ForkProcessParseArkTsStackInfo(const std::string& moduleName, const std::string& path); | 51 | bool ForkProcessParseArkTsStackInfo(const std::string& moduleName, const std::string& path); |
| 49 | 52 | ||