From be6ddb11a271fadfbc854994003e41885c6d2526 Mon Sep 17 00:00:00 2001 From: Dom Del Nano Date: Thu, 3 Oct 2024 18:38:06 +0000 Subject: [PATCH 1/3] Fix bug with resolving relative symlinks during linux header detection Signed-off-by: Dom Del Nano --- src/stirling/utils/linux_headers.cc | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/stirling/utils/linux_headers.cc b/src/stirling/utils/linux_headers.cc index a4e73c8a682..9acd1f92407 100644 --- a/src/stirling/utils/linux_headers.cc +++ b/src/stirling/utils/linux_headers.cc @@ -229,7 +229,23 @@ StatusOr ResolvePossibleSymlinkToHostPath(const std::file return error::Internal(ec.message()); } - const auto resolved_host_path = system::Config::GetInstance().ToHostPath(resolved); + // Relative symlinks can form an invalid path when converted to a host path when there are too + // many "../" references, causing the mounted host path to be lost. + // This happens for openSUSE's linux-header package. In order to handle + // this, we only use the symlink target if it is an absolute path and fall back to using the + // symlink name directly when it's relative path (/host/lib/modules//build). + const std::filesystem::path resolved_host_path = [&resolved, &p]() -> std::filesystem::path { + if (resolved.is_absolute()) { + const auto host_path_from_abs_symlink = system::Config::GetInstance().ToHostPath(resolved); + LOG(INFO) << absl::Substitute( + "Symlink is using absolute path. Converting that to host path: $0 -> $1.", + resolved.string(), host_path_from_abs_symlink.string()); + return host_path_from_abs_symlink; + } + LOG(INFO) << absl::Substitute( + "Symlink is using relative path, using link name directly instead: $0", p.string()); + return p; + }(); // Downstream won't be ok unless the resolved host path exists; return an error if needed. if (!fs::Exists(resolved_host_path)) { From 0322c23b85bcb270c6bf7259f27999fdaaff5999 Mon Sep 17 00:00:00 2001 From: Dom Del Nano Date: Fri, 11 Oct 2024 21:35:40 +0000 Subject: [PATCH 2/3] Resolve relative symlink instead of clobbering it. Remove unnecssary lambda. Signed-off-by: Dom Del Nano --- src/stirling/utils/linux_headers.cc | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/stirling/utils/linux_headers.cc b/src/stirling/utils/linux_headers.cc index 9acd1f92407..039fabdb527 100644 --- a/src/stirling/utils/linux_headers.cc +++ b/src/stirling/utils/linux_headers.cc @@ -229,23 +229,20 @@ StatusOr ResolvePossibleSymlinkToHostPath(const std::file return error::Internal(ec.message()); } - // Relative symlinks can form an invalid path when converted to a host path when there are too - // many "../" references, causing the mounted host path to be lost. - // This happens for openSUSE's linux-header package. In order to handle - // this, we only use the symlink target if it is an absolute path and fall back to using the - // symlink name directly when it's relative path (/host/lib/modules//build). - const std::filesystem::path resolved_host_path = [&resolved, &p]() -> std::filesystem::path { - if (resolved.is_absolute()) { - const auto host_path_from_abs_symlink = system::Config::GetInstance().ToHostPath(resolved); - LOG(INFO) << absl::Substitute( - "Symlink is using absolute path. Converting that to host path: $0 -> $1.", - resolved.string(), host_path_from_abs_symlink.string()); - return host_path_from_abs_symlink; - } + // Relative paths containing "../" can result in an invalid host mount path when using + // ToHostPath. Therefore, we need to treat the absolute and relative cases differently. + std::filesystem::path resolved_host_path = p.parent_path(); + if (resolved.is_absolute()) { + resolved_host_path = system::Config::GetInstance().ToHostPath(resolved); LOG(INFO) << absl::Substitute( - "Symlink is using relative path, using link name directly instead: $0", p.string()); - return p; - }(); + "Symlink target is an absolute path. Converting that to host path: $0 -> $1.", + resolved.string(), resolved_host_path.string()); + } else { + resolved_host_path /= resolved.string(); + LOG(INFO) << absl::Substitute( + "Symlink target is a relative path. Concatenating it to parent directory: $0", + resolved_host_path.string()); + } // Downstream won't be ok unless the resolved host path exists; return an error if needed. if (!fs::Exists(resolved_host_path)) { From ed7b839d630c0ee2acdde1319a008d6d099e0e18 Mon Sep 17 00:00:00 2001 From: Dom Del Nano Date: Fri, 11 Oct 2024 22:02:13 +0000 Subject: [PATCH 3/3] Update logging to VLOG and address other feedback Signed-off-by: Dom Del Nano --- src/stirling/utils/linux_headers.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/stirling/utils/linux_headers.cc b/src/stirling/utils/linux_headers.cc index 039fabdb527..75cbff9fe56 100644 --- a/src/stirling/utils/linux_headers.cc +++ b/src/stirling/utils/linux_headers.cc @@ -231,15 +231,16 @@ StatusOr ResolvePossibleSymlinkToHostPath(const std::file // Relative paths containing "../" can result in an invalid host mount path when using // ToHostPath. Therefore, we need to treat the absolute and relative cases differently. - std::filesystem::path resolved_host_path = p.parent_path(); + std::filesystem::path resolved_host_path; if (resolved.is_absolute()) { resolved_host_path = system::Config::GetInstance().ToHostPath(resolved); - LOG(INFO) << absl::Substitute( + VLOG(1) << absl::Substitute( "Symlink target is an absolute path. Converting that to host path: $0 -> $1.", resolved.string(), resolved_host_path.string()); } else { + resolved_host_path = p.parent_path(); resolved_host_path /= resolved.string(); - LOG(INFO) << absl::Substitute( + VLOG(1) << absl::Substitute( "Symlink target is a relative path. Concatenating it to parent directory: $0", resolved_host_path.string()); }