GNU bug report logs - #78256
[PATCH] daemon: Use the actual overflow UID and GID in /etc/passwd.

Previous Next

To reply to this bug, email your comments to 78256 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: guix-patches <at> gnu.org
Cc: keinflue <keinflue <at> posteo.net>,
 Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH] daemon: Use the actual overflow UID and GID in /etc/passwd.
Date: Mon,  5 May 2025 10:59:34 +0200

Partly fixes <https://issues.guix.gnu.org/77862>.

* nix/libstore/build.cc (fileContent, overflowUID, overflowGID): New
functions.
(DerivationGoal::startBuilder): Use them to populate /etc/passwd when
‘buildUser.enabled()’ is false.

Reported-by: keinflue <keinflue <at> posteo.net>
Change-Id: I695c697629c739d096933274c1c8a70d08468d4a
---
 nix/libstore/build.cc | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index a1f39d9a8b..773dcf1a01 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -13,6 +13,7 @@
 #include <map>
 #include <sstream>
 #include <algorithm>
+#include <iostream>
 
 #include <limits.h>
 #include <time.h>
@@ -1646,6 +1647,36 @@ static void initializeUserNamespace(pid_t child,
 	      (format("%d %d 1") % guestGID % hostGID).str());
 }
 
+/* Return the content of FILE as an integer, or DFLT if FILE could not be
+   opened or parsed.  */
+static unsigned int fileContent(const std::string &file, int dflt)
+{
+    AutoCloseFD fd;
+    fd = open(file.c_str(), O_RDONLY|O_CLOEXEC);
+    if (fd == -1)
+	return dflt;
+    else {
+	char buf[64];
+	ssize_t count = read (fd, buf, sizeof buf);
+	if (count <= 0) return dflt;
+
+	unsigned int result = dflt;
+	std::string str = buf;
+	try { result = std::stoi(str); } catch (...) {};
+	return result;
+    }
+}
+
+static uid_t overflowUID()
+{
+    return fileContent("/proc/sys/kernel/overflowuid", 65534);
+}
+
+static gid_t overflowGID()
+{
+    return fileContent("/proc/sys/kernel/overflowgid", 65534);
+}
+
 void DerivationGoal::startBuilder()
 {
     auto f = format(
@@ -1846,9 +1877,11 @@ void DerivationGoal::startBuilder()
         writeFile(chrootRootDir + "/etc/passwd",
             (format(
                 "nixbld:x:%1%:%2%:Nix build user:/:/noshell\n"
-                "nobody:x:65534:65534:Nobody:/:/noshell\n")
+                "nobody:x:%3%:%4%:Nobody:/:/noshell\n")
                 % (buildUser.enabled() ? buildUser.getUID() : guestUID)
-                % (buildUser.enabled() ? buildUser.getGID() : guestGID)).str());
+                % (buildUser.enabled() ? buildUser.getGID() : guestGID)
+	        % (buildUser.enabled() ? 65534 : overflowUID())
+	        % (buildUser.enabled() ? 65534 : overflowGID())).str());
 
         /* Declare the build user's group so that programs get a consistent
            view of the system (e.g., "id -gn"). */

base-commit: c2c4bc8758616ebc0148e1bce9311a80658ace88
-- 
2.49.0

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: keinflue <keinflue <at> posteo.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: guix-patches <at> gnu.org
Subject: Re: [PATCH] daemon: Use the actual overflow UID and GID in
 /etc/passwd.
Date: Mon, 05 May 2025 10:43:59 +0000

On 05.05.2025 10:59, Ludovic Courtès wrote:
> Partly fixes <https://issues.guix.gnu.org/77862>.
> 
> * nix/libstore/build.cc (fileContent, overflowUID, overflowGID): New
> functions.
> (DerivationGoal::startBuilder): Use them to populate /etc/passwd when
> ‘buildUser.enabled()’ is false.
> 
> Reported-by: keinflue <keinflue <at> posteo.net>
> Change-Id: I695c697629c739d096933274c1c8a70d08468d4a
> ---
>  nix/libstore/build.cc | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
> index a1f39d9a8b..773dcf1a01 100644
> --- a/nix/libstore/build.cc
> +++ b/nix/libstore/build.cc
> @@ -13,6 +13,7 @@
>  #include <map>
>  #include <sstream>
>  #include <algorithm>
> +#include <iostream>
> 
>  #include <limits.h>
>  #include <time.h>
> @@ -1646,6 +1647,36 @@ static void initializeUserNamespace(pid_t child,
>  	      (format("%d %d 1") % guestGID % hostGID).str());
>  }
> 
> +/* Return the content of FILE as an integer, or DFLT if FILE could not 
> be
> +   opened or parsed.  */
> +static unsigned int fileContent(const std::string &file, int dflt)

I think dflt should also be unsigned here? (I don't think POSIX 
specifies signdness of the ids, but they are unsigned on Linux.)

> +{
> +    AutoCloseFD fd;
> +    fd = open(file.c_str(), O_RDONLY|O_CLOEXEC);
> +    if (fd == -1)
> +	return dflt;
> +    else {
> +	char buf[64];
> +	ssize_t count = read (fd, buf, sizeof buf);

I am not sure it can happen in the /proc file system, but generally 
there is no guarantee that this will read the whole file even if it is 
smaller than the buffer size. The read may return with partial result on 
a signal and EINTR may also occur.

> +	if (count <= 0) return dflt;
> +
> +	unsigned int result = dflt;
> +	std::string str = buf;

buf is not null-terminated, but this constructor of std::string requires 
a null-terminated byte string as argument. std::string has another 
constructor that takes a count:

std::string str(buf, count);

> +	try { result = std::stoi(str); } catch (...) {};

std::stoi converts to signed int. It will throw for the upper half of 
valid uids/gids and it will accept negative values. I'd recommend to use 
std::stoll instead and to make result have type signed long long. Then 
at the end of the function it is possible to check the values range if 
desired:

if(result < 0 || result > std::numeric_limits<unsigned int>::max())
    return dlft;
else
    return result;

> +	return result;
> +    }
> +}
> +
> +static uid_t overflowUID()
> +{
> +    return fileContent("/proc/sys/kernel/overflowuid", 65534);
> +}
> +
> +static gid_t overflowGID()
> +{
> +    return fileContent("/proc/sys/kernel/overflowgid", 65534);
> +}
> +
>  void DerivationGoal::startBuilder()
>  {
>      auto f = format(
> @@ -1846,9 +1877,11 @@ void DerivationGoal::startBuilder()
>          writeFile(chrootRootDir + "/etc/passwd",
>              (format(
>                  "nixbld:x:%1%:%2%:Nix build user:/:/noshell\n"
> -                "nobody:x:65534:65534:Nobody:/:/noshell\n")
> +                "nobody:x:%3%:%4%:Nobody:/:/noshell\n")
>                  % (buildUser.enabled() ? buildUser.getUID() : 
> guestUID)
> -                % (buildUser.enabled() ? buildUser.getGID() :
> guestGID)).str());
> +                % (buildUser.enabled() ? buildUser.getGID() : 
> guestGID)
> +	        % (buildUser.enabled() ? 65534 : overflowUID())
> +	        % (buildUser.enabled() ? 65534 : overflowGID())).str());
> 
>          /* Declare the build user's group so that programs get a 
> consistent
>             view of the system (e.g., "id -gn"). */
> 
> base-commit: c2c4bc8758616ebc0148e1bce9311a80658ace88

In general, after some more thoughts about it, I am not really sure that 
the ids of "nobody" must reflect the overflowids. It seems that this 
user/group name has/had multiple different purposes and it is not clear 
to me which one exactly is intended for the build environment.

Best,
keinflue

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.