[lldb] Fix SIGSEGV in `GetPtraceScope()` in `Procfs.cpp` by royitaqi · Pull Request #142224 · llvm/llvm-project

royitaqi · 2025-05-30T22:26:38Z

Symptom

We have seen SIGSEGV like this:

* thread #1, name = 'lldb-server', stop reason = SIGSEGV
    frame #0: 0x00007f39e529c993 libc.so.6`__pthread_kill_internal(signo=11, threadid=<unavailable>) at pthread_kill.c:46:37
    ...
  * frame #5: 0x000056027c94fe48 lldb-server`lldb_private::process_linux::GetPtraceScope() + 72
    frame #6: 0x000056027c92f94f lldb-server`lldb_private::process_linux::NativeProcessLinux::Attach(int) + 1087
    ...

See full stack trace.

This happens on Linux where LLDB doesn't have access to /proc/sys/kernel/yama/ptrace_scope.

A similar error (an unchecked Error) can be reproduced by running the newly added unit test without the fix. See the "Test" section below.

Root cause

GetPtraceScope() (code) has the following if statement:

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)
    return errorCodeToError(ptrace_scope_file.getError());
  ...
}

The intention of the if statement is to check whether the ptrace_scope_file is an Error or not, and return the error if it is. However, the operator* of ErrorOr returns the value that is stored (which is a std::unique_ptr<MemoryBuffer>), so what the if condition actually do is to check if the unique pointer is non-null.

Note that the method ErrorOr::getStorage() (called by ErrorOr::operator *) does assert on whether or not HasError has been set (see ErrorOr.h). However, it seems this wasn't executed, probably because the LLDB was a release build.

Fix

The fix is simply remove the * in the said if statement.

Test

Adding a new unit test: RealPtraceScopeWhenNotExist.

On a Linux machine which doesn't have /proc/sys/kernel/yama/ptrace_scope, run the unit test without the fix reproduces this assertion error about an unchecked Error:

$ tools/lldb/unittests/Process/Linux/ProcessLinuxTests
[==========] Running 5 tests from 1 test suite.
...
[ RUN      ] Perf.RealPtraceScopeWhenNotExist
ProcessLinuxTests: /home/royshi/llvm-sand/external/llvm-project/llvm/include/llvm/Support/ErrorOr.h:236: storage_type *llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>::getStorage() [T = std::unique_ptr<llvm::MemoryBuffer>]: Assertion `!HasError && "Cannot get value when an error exists!"' failed.
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  ProcessLinuxTests 0x0000560e7798d6e5
1  ProcessLinuxTests 0x0000560e7798ddf4
...
Aborted (core dumped)

Run the unit test with the fix show passed result:

$ tools/lldb/unittests/Process/Linux/ProcessLinuxTests
[==========] Running 5 tests from 1 test suite.
...
[ RUN      ] Perf.RealPtraceScopeWhenNotExist
[       OK ] Perf.RealPtraceScopeWhenNotExist (0 ms)
...
[  PASSED  ] 3 tests.

llvmbot · 2025-05-30T22:27:13Z

@llvm/pr-subscribers-lldb

Author: None (royitaqi)

Changes

Symptom

We have seen SIGSEGV like this:

* thread #<!-- -->1, name = 'lldb-server', stop reason = SIGSEGV
    frame #<!-- -->0: 0x00007f39e529c993 libc.so.6`__pthread_kill_internal(signo=11, threadid=&lt;unavailable&gt;) at pthread_kill.c:46:37
    ...
  * frame #<!-- -->5: 0x000056027c94fe48 lldb-server`lldb_private::process_linux::GetPtraceScope() + 72
    frame #<!-- -->6: 0x000056027c92f94f lldb-server`lldb_private::process_linux::NativeProcessLinux::Attach(int) + 1087
    ...

See full stack trace.

A similar error (an unchecked Error) can be reproduced by running the newly added unit test without the fix on a Linux machine which doesn't have /proc/sys/kernel/yama/ptrace_scope. See the "Test" section below.

Root cause

GetPtraceScope() (code) has the following if statement:

llvm::Expected&lt;int&gt; lldb_private::process_linux::GetPtraceScope() {
  ErrorOr&lt;std::unique_ptr&lt;MemoryBuffer&gt;&gt; ptrace_scope_file =
      getProcFile("sys/kernel/yama/ptrace_scope");
  if (!*ptrace_scope_file)
    return errorCodeToError(ptrace_scope_file.getError());
  ...
}

The intention of the if statement is to check whether the ptrace_scope_file is an Error or not, and return the error if it is. However, the operator* of ErrorOr returns the value that is stored (which is a std::unique_ptr<MemoryBuffer>), so what the if condition actually do is to check if the unique pointer is non-null.

Note that the method ErrorOr::getStorage() (called by ErrorOr::operator *) does assert on whether or not HasError has been set (see ErrorOr.h). However, it seems this wasn't executed, probably because the LLDB was a release build.

Fix

The fix is simply remove the * in the said if statement.

Test

Adding a new unit test: RealPtraceScopeWhenNotExist.

On a Linux machine which doesn't have /proc/sys/kernel/yama/ptrace_scope, run the unit test without the fix reproduces this assertion error about an unchecked Error:

$ tools/lldb/unittests/Process/Linux/ProcessLinuxTests
[==========] Running 5 tests from 1 test suite.
...
[ RUN      ] Perf.RealPtraceScopeWhenNotExist
ProcessLinuxTests: /home/royshi/llvm-sand/external/llvm-project/llvm/include/llvm/Support/ErrorOr.h:236: storage_type *llvm::ErrorOr&lt;std::unique_ptr&lt;llvm::MemoryBuffer&gt;&gt;::getStorage() [T = std::unique_ptr&lt;llvm::MemoryBuffer&gt;]: Assertion `!HasError &amp;&amp; "Cannot get value when an error exists!"' failed.
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  ProcessLinuxTests 0x0000560e7798d6e5
1  ProcessLinuxTests 0x0000560e7798ddf4
...
Aborted (core dumped)

Run the unit test with the fix show passed result:

[royshi@<!-- -->devvm24317.cln0 ~/llvm-sand/build/Debug/fbcode-x86_64/toolchain]$ tools/lldb/unittests/Process/Linux/ProcessLinuxTests
[==========] Running 5 tests from 1 test suite.
...
[ RUN      ] Perf.RealPtraceScopeWhenNotExist
[       OK ] Perf.RealPtraceScopeWhenNotExist (0 ms)
...
[  PASSED  ] 3 tests.

Full diff: https://github.com/llvm/llvm-project/pull/142224.diff

2 Files Affected:

(modified) lldb/source/Plugins/Process/Linux/Procfs.cpp (+1-1)
(modified) lldb/unittests/Process/Linux/ProcfsTests.cpp (+15-1)

diff --git a/lldb/source/Plugins/Process/Linux/Procfs.cpp b/lldb/source/Plugins/Process/Linux/Procfs.cpp
index d3bd396fbaeab..4349a2b1adb9d 100644
--- a/lldb/source/Plugins/Process/Linux/Procfs.cpp
+++ b/lldb/source/Plugins/Process/Linux/Procfs.cpp
@@ -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)
     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();
diff --git a/lldb/unittests/Process/Linux/ProcfsTests.cpp b/lldb/unittests/Process/Linux/ProcfsTests.cpp
index e7af1f469c2bf..a795fa4e019c5 100644
--- a/lldb/unittests/Process/Linux/ProcfsTests.cpp
+++ b/lldb/unittests/Process/Linux/ProcfsTests.cpp
@@ -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) {
   // We first check we can read /proc/sys/kernel/yama/ptrace_scope
   auto buffer_or_error =
       errorOrToExpected(getProcFile("sys/kernel/yama/ptrace_scope"));
@@ -120,6 +120,20 @@ 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");

github-actions · 2025-05-30T22:29:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Jlalond · 2025-05-30T22:33:02Z

lldb/source/Plugins/Process/Linux/Procfs.cpp

  ErrorOr<std::unique_ptr<MemoryBuffer>> ptrace_scope_file =
      getProcFile("sys/kernel/yama/ptrace_scope");
-  if (!*ptrace_scope_file)
+  if (!ptrace_scope_file)


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

The first checks the Error, the second the unique_ptr.

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.

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"

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

Jlalond · 2025-05-30T22:34:41Z

lldb/unittests/Process/Linux/ProcfsTests.cpp

 }

-TEST(Perf, RealPtraceScope) {
+TEST(Perf, RealPtraceScopeWhenExist) {


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.

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.

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

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.

Jlalond

LGTM

labath · 2025-06-02T09:10:20Z

lldb/source/Plugins/Process/Linux/Procfs.cpp

  ErrorOr<std::unique_ptr<MemoryBuffer>> ptrace_scope_file =
      getProcFile("sys/kernel/yama/ptrace_scope");
-  if (!*ptrace_scope_file)
+  if (!ptrace_scope_file)


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"

royitaqi · 2025-06-02T15:08:29Z

Updated according to @labath 's comment.

@JDevlieghere / @clayborg: Gentle ping - Do you guys want to review, or should I go ahead and land with @labath 's approval?

lldb/unittests/Process/Linux/ProcfsTests.cpp

rupprecht

Thank you!

rupprecht · 2025-06-02T17:34:48Z

lldb/unittests/Process/Linux/ProcfsTests.cpp

 }

-TEST(Perf, RealPtraceScope) {
+TEST(Perf, RealPtraceScopeWhenExist) {


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.

royitaqi added 2 commits May 30, 2025 14:21

Fix crash and add test

58b8a32

Fix test

02a2ba2

royitaqi requested a review from JDevlieghere as a code owner May 30, 2025 22:26

llvmbot added the lldb label May 30, 2025

royitaqi requested review from Jlalond, clayborg, dmpots and jeffreytan81 May 30, 2025 22:27

Jlalond reviewed May 30, 2025

View reviewed changes

Jlalond requested changes May 30, 2025

View reviewed changes

royitaqi added 2 commits May 30, 2025 15:55

Add if for unique_ptr

02e9d8b

Fix format

e55bbbe

royitaqi requested review from Jlalond and rupprecht May 30, 2025 23:05

Jlalond approved these changes May 30, 2025

View reviewed changes

labath approved these changes Jun 2, 2025

View reviewed changes

Remove the newly added if

6dcf157

clayborg reviewed Jun 2, 2025

View reviewed changes

lldb/unittests/Process/Linux/ProcfsTests.cpp Show resolved Hide resolved

clayborg approved these changes Jun 2, 2025

View reviewed changes

rupprecht approved these changes Jun 2, 2025

View reviewed changes

royitaqi merged commit 8e8da88 into llvm:main Jun 2, 2025
10 checks passed

royitaqi deleted the fix-crash branch August 5, 2025 16:19

Conversation

royitaqi commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Symptom

Root cause

Fix

Test

Uh oh!

llvmbot commented May 30, 2025

Symptom

Root cause

Fix

Test

Uh oh!

github-actions bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royitaqi May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jlalond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

royitaqi commented Jun 2, 2025

Uh oh!

Uh oh!

rupprecht left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

royitaqi commented May 30, 2025 •

edited

Loading

github-actions bot commented May 30, 2025 •

edited

Loading

royitaqi May 30, 2025 •

edited

Loading