Disable building subids support on systems that don't have the prctl

[sthibaul]

No description provided.

[alejandro-colomar]

Cc: @thesamesam

[lslebodn]

@sthibaul would you be so kind and could you a bit more elaborate about "systems that don't have the prctl" ?

[sthibaul]

@sthibaul would you be so kind and could you a bit more elaborate about "systems that don't have the prctl" ?

prctl is not in posix. AFAIK it exists only on Linux and IRIX, so it's essentially all other systems. The case that matters to me is GNU/Hurd.

[lslebodn]

@sthibaul a bit offtopic to your PR.

prctl is not used always. It is compiled conditionally if header <sys/capability.h> can be included

• shadow/lib/idmapping.c

  Lines 132 to 150 in c44f1e0

  	#if __has_include(<sys/capability.h>)
  	int cap;
  	struct __user_cap_header_struct hdr = {_LINUX_CAPABILITY_VERSION_3, 0};
  	struct __user_cap_data_struct data[2] = {{0}};
  	if (streq(map_file, "uid_map")) {
  	cap = CAP_SETUID;
  	} else if (streq(map_file, "gid_map")) {
  	cap = CAP_SETGID;
  	} else {
  	fprintf(log_get_logfd(), _("%s: Invalid map file %s specified\n"), log_get_progname(), map_file);
  	exit(EXIT_FAILURE);
  	}
  	/* Align setuid- and fscaps-based new{g,u}idmap behavior. */
  	if (geteuid() == 0 && geteuid() != ruid) {
  	if (prctl(PR_SET_KEEPCAPS, 1L) == -1) {
  	fprintf(log_get_logfd(), _("%s: Could not prctl(PR_SET_KEEPCAPS)\n"), log_get_progname());
  	exit(EXIT_FAILURE);

But it is included always, even though it will not be used

• shadow/lib/idmapping.c

  Lines 15 to 18 in c44f1e0

  	#include <sys/prctl.h>
  	#if __has_include(<sys/capability.h>)
  	# include <sys/capability.h>
  	#endif

I wonder whether you would be able to compile it on GNU/Hurd with following diff

diff --git a/lib/idmapping.c b/lib/idmapping.c
index e70a037b..3425e1bb 100644
--- a/lib/idmapping.c
+++ b/lib/idmapping.c
@@ -12,8 +12,8 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <strings.h>
-#include <sys/prctl.h>
 #if __has_include(<sys/capability.h>)
+#include <sys/prctl.h>
 # include <sys/capability.h>
 #endif

It would be almost a revert of f50a39e

commit f50a39e8f9a00c1c2b6b26a80448d510c56353bb
Author: Alejandro Colomar <alx@kernel.org>
Date:   Tue Nov 12 14:42:12 2024 +0100

    lib/idmapping.c: Unconditionally include <sys/prctl.h>
    
    It's a widely available header.
    
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

diff --git a/lib/idmapping.c b/lib/idmapping.c
index 89c03b10..cef73b4f 100644
--- a/lib/idmapping.c
+++ b/lib/idmapping.c
@@ -12,8 +12,8 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <strings.h>
-#if HAVE_SYS_CAPABILITY_H
 #include <sys/prctl.h>
+#if HAVE_SYS_CAPABILITY_H
 #include <sys/capability.h>
 #endif

[alejandro-colomar]

@sthibaul a bit offtopic to your PR.

prctl is not used always. It is compiled conditionally if header <sys/capability.h> can be included

* https://github.com/shadow-maint/shadow/blob/c44f1e096a19a7d356da5969295393247e61874f/lib/idmapping.c#L132-L150

But it is included always, even though it will not be used

* https://github.com/shadow-maint/shadow/blob/c44f1e096a19a7d356da5969295393247e61874f/lib/idmapping.c#L15-L18

I wonder whether you would be able to compile it on GNU/Hurd with following diff

diff --git a/lib/idmapping.c b/lib/idmapping.c
index e70a037b..3425e1bb 100644
--- a/lib/idmapping.c
+++ b/lib/idmapping.c
@@ -12,8 +12,8 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <strings.h>
-#include <sys/prctl.h>
 #if __has_include(<sys/capability.h>)
+#include <sys/prctl.h>
 # include <sys/capability.h>
 #endif

It would be almost a revert of f50a39e

Shouldn't we use #if __has_include(<sys/prctl.h>)?

[sthibaul]

@sthibaul a bit offtopic to your PR.

prctl is not used always. It is compiled conditionally if header <sys/capability.h> can be included

* https://github.com/shadow-maint/shadow/blob/c44f1e096a19a7d356da5969295393247e61874f/lib/idmapping.c#L132-L150

But it is included always, even though it will not be used

* https://github.com/shadow-maint/shadow/blob/c44f1e096a19a7d356da5969295393247e61874f/lib/idmapping.c#L15-L18

I wonder whether you would be able to compile it on GNU/Hurd with following diff

diff --git a/lib/idmapping.c b/lib/idmapping.c
index e70a037b..3425e1bb 100644
--- a/lib/idmapping.c
+++ b/lib/idmapping.c
@@ -12,8 +12,8 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <strings.h>
-#include <sys/prctl.h>
 #if __has_include(<sys/capability.h>)
+#include <sys/prctl.h>
 # include <sys/capability.h>
 #endif

That does make the build work indeed. Not sure it will be future-proof, though.

[alejandro-colomar]

LGTM. Thanks!

Reviewed-by: Alejandro Colomar <alx@kernel.org>

[lslebodn]

That does make the build work indeed. Not sure it will be future-proof, though.

Hopefully one would check git blame before "cleanup" similar as in f50a39e :)

🤞

commit f50a39e8f9a00c1c2b6b26a80448d510c56353bb
Author: Alejandro Colomar <alx@kernel.org>
Date:   Tue Nov 12 14:42:12 2024 +0100

    lib/idmapping.c: Unconditionally include <sys/prctl.h>
    
    It's a widely available header.
    
    Signed-off-by: Alejandro Colomar <alx@kernel.org>

diff --git a/lib/idmapping.c b/lib/idmapping.c
index 89c03b10..cef73b4f 100644
--- a/lib/idmapping.c
+++ b/lib/idmapping.c
@@ -12,8 +12,8 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <strings.h>
-#if HAVE_SYS_CAPABILITY_H
 #include <sys/prctl.h>
+#if HAVE_SYS_CAPABILITY_H
 #include <sys/capability.h>
 #endif