Skip to content

[lldb] Fix SIGSEGV in GetPtraceScope() in Procfs.cpp #142224

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
merged 5 commits into from
Jun 2, 2025
Merged

[lldb] Fix SIGSEGV in GetPtraceScope() in Procfs.cpp #142224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58b8a32
Fix crash and add test
royitaqi May 30, 2025
02a2ba2
Fix test
royitaqi May 30, 2025
02e9d8b
Add if for unique_ptr
royitaqi May 30, 2025
e55bbbe
Fix format
royitaqi May 30, 2025
6dcf157
Remove the newly added if
royitaqi Jun 2, 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
2 changes: 1 addition & 1 deletion lldb/source/Plugins/Process/Linux/Procfs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ lldb_private::process_linux::GetAvailableLogicalCoreIDs() {
llvm::Expected<int> lldb_private::process_linux::GetPtraceScope() {
ErrorOr<std::unique_ptr<MemoryBuffer>> ptrace_scope_file =
getProcFile("sys/kernel/yama/ptrace_scope");
if (!*ptrace_scope_file)
if (!ptrace_scope_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to do both, !ptrace_scope_file || !*ptrace_scope_file.

The first checks the Error, the second the unique_ptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2nd one has to be in a different if statement, because ptrace_scope_file.getError() would have no error.

Adding this in the latest commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we do. The default for functions returning Expected/ErrorOr<some_ptr<Foo>> is that they return a valid object in the non-error case. If they aren't they should very explicitly document what it means to "successfully return nothing"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the newly added if per @labath 's comment.

return errorCodeToError(ptrace_scope_file.getError());
// The contents should be something like "1\n". Trim it so we get "1".
StringRef buffer = (*ptrace_scope_file)->getBuffer().trim();
Expand Down
17 changes: 16 additions & 1 deletion lldb/unittests/Process/Linux/ProcfsTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ TEST(Perf, RealLogicalCoreIDs) {
ASSERT_GT((int)cpu_ids->size(), 0) << "We must see at least one core";
}

TEST(Perf, RealPtraceScope) {
TEST(Perf, RealPtraceScopeWhenExist) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will probably fail, I don't think kernel/yama is guaranteed to exist on ALL Linux hosts. We should either create it or not.

I think this original code assumed if you ever got an EPERM from ATTACH you would always have this file.

Copy link
Contributor Author

@royitaqi royitaqi May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This will probably fail

I don't think so. Look at line 112. It skips the test if the file doesn't exist.

We should either create it or not.

I am not sure if we want a unit test to create files that may or may not be depended on by other tests, etc.

Given how the original code was written (hardcoded path), the current setup (skip one test or the other) seems to be reasonable. I agree it's not ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @rupprecht (who added the original unit test) in case they have additional thoughts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's a bug that I wrote if (!*ptrace_scope_file) instead of if (!ptrace_scope_file) -- I don't think there was anything I had in mind when writing it that way, e.g. if we should be able to assume the ptrace scope file exists & is readable. I'm surprised nobody noticed this bug earlier.

// We first check we can read /proc/sys/kernel/yama/ptrace_scope
auto buffer_or_error =
errorOrToExpected(getProcFile("sys/kernel/yama/ptrace_scope"));
Expand All @@ -120,6 +120,21 @@ TEST(Perf, RealPtraceScope) {
<< "Sensible values of ptrace_scope are between 0 and 3";
}

TEST(Perf, RealPtraceScopeWhenNotExist) {
// We first check we can NOT read /proc/sys/kernel/yama/ptrace_scope
auto buffer_or_error =
errorOrToExpected(getProcFile("sys/kernel/yama/ptrace_scope"));
if (buffer_or_error)
GTEST_SKIP() << "In order for this test to run, "
"/proc/sys/kernel/yama/ptrace_scope should not exist";
consumeError(buffer_or_error.takeError());

// At this point we should fail parsing the ptrace_scope value.
Expected<int> ptrace_scope = GetPtraceScope();
ASSERT_FALSE((bool)ptrace_scope);
consumeError(ptrace_scope.takeError());
}

#ifdef LLVM_ENABLE_THREADING
TEST(Support, getProcFile_Tid) {
auto BufferOrError = getProcFile(getpid(), llvm::get_threadid(), "comm");
Expand Down
Loading