[Lldb-commits] [PATCH] D33778: Add a NativeProcessProtocol Factory class

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:112
+  if (wpid != pid || !WIFSTOPPED(wstatus)) {
+    std::error_code EC(errno, std::generic_category());
+    LLDB_LOG(
----------------
krytarowski wrote:
> I can imagine that in some cases there might be returned `errno=0` and eg. `WIFSIGNALED()=true`. `wstatus` wouldn't be readable for end-user - I think it's better to check for `(wpid != -1)` following the same code between `{}` and add dedicated code for `(wpid != pid || !WIFSTOPPED(wstatus))` that the state is unexpected and terminating.
After further consideration, I believe that waitpid will always succeed because the process launcher will make sure for us that the process has actually started (so I changed the wpid == -1 part into an assert). The only part that can fail here is the WIFSTOPPED, which can happen if someone kills the child process before we manage to stop it (in which case we will get a WIFEXITED/WIFSIGNALED instead.


================
Comment at: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:133
 
-  return error;
+  status = process_sp->ReinitializeThreads();
+  if (status.Fail())
----------------
krytarowski wrote:
> ReinitializeThreads() was designed to not set stopped reason in threads.
> 
> ```
>   for (const auto &thread_sp : m_threads) {
>     static_pointer_cast<NativeThreadNetBSD>(thread_sp)->SetStoppedBySignal(
>         SIGSTOP);
> }
> ```
> 
> I think we need the equivalent of this code there.
> 
> Similar for:
> 
> 
> ```
> /* Set process stopped */
> SetState(StateType::eStateStopped);
> ```
That sounds correct. I have the equivalent code in the attach case, I must have missed this one somehow.


================
Comment at: source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:834
+Status NativeProcessNetBSD::Attach() {
+  if (m_pid <= 1)
+    return Status("Attaching to process 1 is not allowed.");
----------------
krytarowski wrote:
> Technically it is possible on NetBSD, with uid=root and in kernel in INSECURE mode. This way root can debug `init`. In all other cases `ptrace`(2) returns error for PID 1.
> 
> I think it should be allowed, or rather not explicitly prohibited behavior.
Agreed. In fact, I have removed the same check from linux code. If the user has enough privileges, and really wants to debug init, I don't think we should stop him.


https://reviews.llvm.org/D33778