Skip to content

Add PEM command line flag for enforcing a max file size for ElfReader #2194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ddelnano merged 2 commits into from
May 14, 2025
Merged

Add PEM command line flag for enforcing a max file size for ElfReader #2194

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6bb573
Add PEM command line flag for enforcing a max file size for ElfReader
ddelnano May 6, 2025
9d48a38
Refactor file size logic to fs_wrapper function
ddelnano May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 54 additions & 29 deletions src/stirling/obj_tools/elf_reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@
#include "src/common/fs/fs_wrapper.h"
#include "src/stirling/obj_tools/init.h"

DEFINE_int64(elf_reader_max_file_size, 0,
"Maximum file size in bytes for ELF files. Default value of 0 means all files will "
"be parsed regardless of size.");

namespace px {
namespace stirling {
namespace obj_tools {
Expand All @@ -46,6 +50,41 @@ struct LowercaseHex {
};
} // namespace

StatusOr<std::unique_ptr<ElfReader>> ElfReader::CreateImpl(
const std::string& binary_path, const std::filesystem::path& debug_file_dir) {
VLOG(1) << absl::Substitute("Creating ElfReader, [binary=$0] [debug_file_dir=$1]", binary_path,
debug_file_dir.string());

auto elf_reader = std::unique_ptr<ElfReader>(new ElfReader);
elf_reader->binary_path_ = binary_path;

if (!elf_reader->elf_reader_.load_header_and_sections(binary_path)) {
return error::Internal("Can't find or process ELF file $0", binary_path);
}

// Check for external debug symbols.
Status s = elf_reader->LocateDebugSymbols(debug_file_dir);
if (s.ok()) {
std::string debug_symbols_path = elf_reader->debug_symbols_path_.string();

bool internal_debug_symbols =
fs::Equivalent(elf_reader->debug_symbols_path_, binary_path).ConsumeValueOr(true);

// If external debug symbols were found, load that ELF info instead.
if (!internal_debug_symbols) {
std::string debug_symbols_path = elf_reader->debug_symbols_path_.string();
LOG(INFO) << absl::Substitute("Found debug symbols file $0 for binary $1", debug_symbols_path,
binary_path);
elf_reader->elf_reader_.load_header_and_sections(debug_symbols_path);
return elf_reader;
}
}

// Debug symbols were either in the binary, or no debug symbols were found,
// so return original elf_reader.
return elf_reader;
}

Status ElfReader::LocateDebugSymbols(const std::filesystem::path& debug_file_dir) {
std::string build_id;
std::string debug_link;
Expand Down Expand Up @@ -158,40 +197,26 @@ Status ElfReader::LocateDebugSymbols(const std::filesystem::path& debug_file_dir
return error::Internal("Could not find debug symbols for $0", binary_path_);
}

// TODO(oazizi): Consider changing binary_path to std::filesystem::path.
StatusOr<std::unique_ptr<ElfReader>> ElfReader::Create(
const std::string& binary_path, const std::filesystem::path& debug_file_dir) {
VLOG(1) << absl::Substitute("Creating ElfReader, [binary=$0] [debug_file_dir=$1]", binary_path,
debug_file_dir.string());
auto elf_reader = std::unique_ptr<ElfReader>(new ElfReader);

elf_reader->binary_path_ = binary_path;

if (!elf_reader->elf_reader_.load_header_and_sections(binary_path)) {
return error::Internal("Can't find or process ELF file $0", binary_path);
}

// Check for external debug symbols.
Status s = elf_reader->LocateDebugSymbols(debug_file_dir);
if (s.ok()) {
std::string debug_symbols_path = elf_reader->debug_symbols_path_.string();

bool internal_debug_symbols =
fs::Equivalent(elf_reader->debug_symbols_path_, binary_path).ConsumeValueOr(true);

// If external debug symbols were found, load that ELF info instead.
if (!internal_debug_symbols) {
std::string debug_symbols_path = elf_reader->debug_symbols_path_.string();
LOG(INFO) << absl::Substitute("Found debug symbols file $0 for binary $1", debug_symbols_path,
binary_path);
elf_reader->elf_reader_.load_header_and_sections(debug_symbols_path);
return elf_reader;
if (FLAGS_elf_reader_max_file_size != 0) {
PX_ASSIGN_OR_RETURN(const auto stat, fs::Stat(binary_path));
int64_t file_size = stat.st_size;
if (file_size > FLAGS_elf_reader_max_file_size) {
return error::Internal(
"File size $0 exceeds ElfReader's max file size $1. Refusing to process file", file_size,
FLAGS_elf_reader_max_file_size);
} else if (file_size < 0) {
return error::Internal("stat() returned negative file size $0 for file $1", file_size,
binary_path);
Copy link
Copy Markdown
Contributor

@oazizi000 oazizi000 May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put much of this in a helper function StatusOr GetFileSize(), which can return an error in the case of file_size < 0. Then you can do:

 PX_ASSIGN_OR_RETURN(int64_t file_size, GetFileSize(binary_path));
 if (file_size > FLAGS_elf_reader_max_file_size) {
    return error::Internal(
          "File size $0 exceeds ElfReader's max file size $1. Refusing to process file", file_size,
          FLAGS_elf_reader_max_file_size);
  }

which is a bit more focused.

Copy link
Copy Markdown
Member Author

@ddelnano ddelnano May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I've refactored this and relocated to the fs_wrapper code in 9d48a38.

}
}
return CreateImpl(binary_path, debug_file_dir);
}

// Debug symbols were either in the binary, or no debug symbols were found,
// so return original elf_reader.
return elf_reader;
StatusOr<std::unique_ptr<ElfReader>> ElfReader::CreateUncapped(
const std::string& binary_path, const std::filesystem::path& debug_file_dir) {
return CreateImpl(binary_path, debug_file_dir);
}

StatusOr<ELFIO::section*> ElfReader::SymtabSection() {
Expand Down
15 changes: 13 additions & 2 deletions src/stirling/obj_tools/elf_reader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ namespace px {
namespace stirling {
namespace obj_tools {

constexpr std::string_view kDebugFileDir = "/usr/lib/debug";

class ElfReader {
public:
/**
Expand All @@ -50,8 +52,14 @@ class ElfReader {
* @return error if could not setup elf reader.
*/
static StatusOr<std::unique_ptr<ElfReader>> Create(
const std::string& binary_path,
const std::filesystem::path& debug_file_dir = "/usr/lib/debug");
const std::string& binary_path, const std::filesystem::path& debug_file_dir = kDebugFileDir);

/**
* Creates an ElfReader that does not enforce the max file size limit. This is useful for cases
* where the binary size is known in advance or the binary must be loaded regardless of size.
*/
static StatusOr<std::unique_ptr<ElfReader>> CreateUncapped(
const std::string& binary_path, const std::filesystem::path& debug_file_dir = kDebugFileDir);
Comment on lines +55 to +62
Copy link
Copy Markdown
Contributor

@oazizi000 oazizi000 May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an API perspective, it might be simpler to have a single Create() where the size is an optional parameter that defaults to FLAGS_elf_reader_max_file_size. What do you think?

Copy link
Copy Markdown
Member Author

@ddelnano ddelnano May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to changing this, but I wanted to make the uncapped behavior an explicit opt-in for consumers of ElfReader. Since it's used in many places, I could see a future bug where the max file size check is unintentionally bypassed when most consumers should retain the restriction.

Copy link
Copy Markdown
Contributor

@oazizi000 oazizi000 May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this offline, but the suggestion is that the optional parameter defaults to the capped version, so I don't think it exposes any risk. The goal is just to keep the API simpler. But this is a bit subjective, so doesn't need to be a blocker if you prefer the two separate API calls.


std::filesystem::path& debug_symbols_path() { return debug_symbols_path_; }

Expand Down Expand Up @@ -207,6 +215,9 @@ class ElfReader {
}

private:
static StatusOr<std::unique_ptr<ElfReader>> CreateImpl(
const std::string& binary_path, const std::filesystem::path& debug_file_dir);

ElfReader() = default;

StatusOr<ELFIO::section*> SymtabSection();
Expand Down
4 changes: 3 additions & 1 deletion src/stirling/source_connectors/socket_tracer/socket_trace_bpf_tables.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ ConnInfoMapManager::ConnInfoMapManager(bpf_tools::BCCWrapper* bcc)
: conn_info_map_(WrappedBCCMap<uint64_t, struct conn_info_t>::Create(bcc, "conn_info_map")),
conn_disabled_map_(WrappedBCCMap<uint64_t, uint64_t>::Create(bcc, "conn_disabled_map")) {
std::filesystem::path self_path = GetSelfPath().ValueOrDie();
auto elf_reader_or_s = obj_tools::ElfReader::Create(self_path.string());
// Opt out of the max file size restriction as this is a self-probe and must attach for
// stirling to work.
auto elf_reader_or_s = obj_tools::ElfReader::CreateUncapped(self_path.string());
if (!elf_reader_or_s.ok()) {
LOG(FATAL) << "Failed to create ElfReader for self probe";
}
Expand Down