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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
