diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 61f726bf9..5af385aed 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -84,22 +84,8 @@ void DerivationBuildingGoal::killChild() hook.reset(); #endif #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows - if (builder && builder->pid != -1) { + if (builder && builder->killChild()) worker.childTerminated(this); - - // FIXME: move this into DerivationBuilder. - - /* If we're using a build user, then there is a tricky race - condition: if we kill the build user before the child has - done its setuid() to the build user uid, then it won't be - killed, and we'll potentially lock up in pid.wait(). So - also send a conventional kill to the child. */ - ::kill(-builder->pid, SIGKILL); /* ignore the result */ - - builder->killSandbox(true); - - builder->pid.wait(); - } #endif } diff --git a/src/libstore/include/nix/store/build/derivation-builder.hh b/src/libstore/include/nix/store/build/derivation-builder.hh index 65d044a79..3e8903e8a 100644 --- a/src/libstore/include/nix/store/build/derivation-builder.hh +++ b/src/libstore/include/nix/store/build/derivation-builder.hh @@ -193,10 +193,12 @@ struct DerivationBuilder : RestrictionContext virtual SingleDrvOutputs unprepareBuild() = 0; /** - * Kill any processes running under the build user UID or in the - * cgroup of the build. + * Forcibly kill the child process, if any. + * + * @returns whether the child was still alive and needed to be + * killed. */ - virtual void killSandbox(bool getStats) = 0; + virtual bool killChild() = 0; }; #ifndef _WIN32 // TODO enable `DerivationBuilder` on Windows diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 4678dae42..dff7d0eaa 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -370,9 +370,15 @@ protected: */ virtual void cleanupBuild(bool force); + /** + * Kill any processes running under the build user UID or in the + * cgroup of the build. + */ + virtual void killSandbox(bool getStats); + public: - void killSandbox(bool getStats) override; + bool killChild() override; private: @@ -435,6 +441,24 @@ void DerivationBuilderImpl::killSandbox(bool getStats) } } +bool DerivationBuilderImpl::killChild() +{ + bool ret = pid != -1; + if (ret) { + /* If we're using a build user, then there is a tricky race + condition: if we kill the build user before the child has + done its setuid() to the build user uid, then it won't be + killed, and we'll potentially lock up in pid.wait(). So + also send a conventional kill to the child. */ + ::kill(-pid, SIGKILL); /* ignore the result */ + + killSandbox(true); + + pid.wait(); + } + return ret; +} + bool DerivationBuilderImpl::prepareBuild() { if (useBuildUsers()) {