[PATCH] pasta.c: modify hostname when detaching new namespace

[PATCH] pasta.c: modify hostname when detaching new namespace

[Danish Prakash]

When invoking pasta without any arguments, it's difficult
to tell whether we are in the new namespace or not leaving
users a bit confused. This change modifies the host namespace
to add a prefix "pasta-" to make it a bit more obvious.

Signed-off-by: Danish Prakash <contact@danishpraka.sh>
---
 pasta.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/pasta.c b/pasta.c
index 31e1e00..840a2b1 100644
--- a/pasta.c
+++ b/pasta.c
@@ -180,6 +180,8 @@ static int pasta_spawn_cmd(void *arg)
 {
 	const struct pasta_spawn_cmd_arg *a;
 	sigset_t set;
+	char hostname[HOST_NAME_MAX+1], pasta_hostname[HOST_NAME_MAX+1];
+	char *hostname_prefix = "pasta-";
 
 	/* We run in a detached PID and mount namespace: mount /proc over */
 	if (mount("", "/proc", "proc", 0, NULL))
@@ -188,6 +190,17 @@ static int pasta_spawn_cmd(void *arg)
 	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
 		warn("Cannot set ping_group_range, ICMP requests might fail");
 
+	if (gethostname(hostname, HOST_NAME_MAX+1) == 0) {
+		if ((strlen(hostname) + strlen(hostname_prefix)) > HOST_NAME_MAX) {
+			hostname[strlen(hostname)-strlen(hostname_prefix)] = '\0';
+		}
+		sprintf(pasta_hostname, "%s%s", hostname_prefix, hostname);
+
+		if (sethostname(pasta_hostname, strlen(pasta_hostname)) != 0) {
+			warn("Unable to set pasta-prefixed hostname");
+		}
+	}
+
 	/* Wait for the parent to be ready: see main() */
 	sigemptyset(&set);
 	sigaddset(&set, SIGUSR1);
-- 
@@ -180,6 +180,8 @@ static int pasta_spawn_cmd(void *arg)
 {
 	const struct pasta_spawn_cmd_arg *a;
 	sigset_t set;
+	char hostname[HOST_NAME_MAX+1], pasta_hostname[HOST_NAME_MAX+1];
+	char *hostname_prefix = "pasta-";
 
 	/* We run in a detached PID and mount namespace: mount /proc over */
 	if (mount("", "/proc", "proc", 0, NULL))
@@ -188,6 +190,17 @@ static int pasta_spawn_cmd(void *arg)
 	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
 		warn("Cannot set ping_group_range, ICMP requests might fail");
 
+	if (gethostname(hostname, HOST_NAME_MAX+1) == 0) {
+		if ((strlen(hostname) + strlen(hostname_prefix)) > HOST_NAME_MAX) {
+			hostname[strlen(hostname)-strlen(hostname_prefix)] = '\0';
+		}
+		sprintf(pasta_hostname, "%s%s", hostname_prefix, hostname);
+
+		if (sethostname(pasta_hostname, strlen(pasta_hostname)) != 0) {
+			warn("Unable to set pasta-prefixed hostname");
+		}
+	}
+
 	/* Wait for the parent to be ready: see main() */
 	sigemptyset(&set);
 	sigaddset(&set, SIGUSR1);
-- 
2.45.1

Re: [PATCH] pasta.c: modify hostname when detaching new namespace

[Danish Prakash]

Thanks for the review and the links.

> +       if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX))) {
> +               if (sethostname(hostname, strlen(hostname)))
> +                       debug("Unable to set pasta-prefixed hostname");
>         }

The above snippet, although it looks correct, doesn't work in cases
where the hostname is long enough (~>58 chars). It works fine for
shorter hostnames. The call to `gethostname()` sets errno to 36
(ENAMETOOLONG). Upon looking further, it seems that even though the
manpage for `gethostname(char *name, size_t len)` states..

>  If the null-terminated hostname is too large to fit, then the name is truncated, and no error is returned (but see NOTES below).

...It would still throw ENAMETOOLONG if the value of len is smaller
than `strlen(hostname)+1` (referring to the snippet below). I'm not
sure if I'm missing out on an edge case while calculating the
boundaries here because the `memcpy` call is certainly truncating the
hostname[1] before ending up returning ENAMETOOLONG, which seems
conflicting[1]:

> int
> __gethostname (char *name, size_t len)
> {
>   struct utsname buf;
>   size_t node_len;
>   if (__uname (&buf))
>     return -1;
>   node_len = strlen (buf.nodename) + 1;
>   memcpy (name, buf.nodename, len < node_len ? len : node_len);
>   if (node_len > len)
>     {
>       __set_errno (ENAMETOOLONG);
>       return -1;
>     }
>   return 0;
> }

[1] - https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=sysdeps/posix/gethostname.c;hb=HEAD


On Tue, 21 May 2024 at 23:04, Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Mon, 20 May 2024 14:05:58 +0530
> Danish Prakash <contact@danishpraka.sh> wrote:
>
> > When invoking pasta without any arguments, it's difficult
> > to tell whether we are in the new namespace or not leaving
> > users a bit confused. This change modifies the host namespace
> > to add a prefix "pasta-" to make it a bit more obvious.
>
> Thanks for the patch, it works for me:
>
> sbrivio@maja:~/passt$ ./pasta --config-net
> root@pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-maja:~/passt# ./pasta --config-net
> root@pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-pasta-past:~/passt#
>
> a few comments (all about brevity and style), below:
>
> > Signed-off-by: Danish Prakash <contact@danishpraka.sh>
> > ---
> >  pasta.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/pasta.c b/pasta.c
> > index 31e1e00..840a2b1 100644
> > --- a/pasta.c
> > +++ b/pasta.c
> > @@ -180,6 +180,8 @@ static int pasta_spawn_cmd(void *arg)
> >  {
> >       const struct pasta_spawn_cmd_arg *a;
> >       sigset_t set;
> > +     char hostname[HOST_NAME_MAX+1], pasta_hostname[HOST_NAME_MAX+1];
>
> For coding style consistency: "HOST_NAME_MAX + 1" (with spaces).
>
> > +     char *hostname_prefix = "pasta-";
>
> In passt, which follows the coding style of the Linux kernel for the
> networking part, we order variables from the longest to the shortest.
> Rationale:
>
>   https://hisham.hm/2018/06/16/when-listing-repeated-things-make-pyramids/
>
> see also https://lwn.net/Articles/758552/.
>
> But actually, you don't need more than one variable here, see below.
>
> >
> >       /* We run in a detached PID and mount namespace: mount /proc over */
> >       if (mount("", "/proc", "proc", 0, NULL))
> > @@ -188,6 +190,17 @@ static int pasta_spawn_cmd(void *arg)
> >       if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
> >               warn("Cannot set ping_group_range, ICMP requests might fail");
> >
> > +     if (gethostname(hostname, HOST_NAME_MAX+1) == 0) {
>
> In general, unless one wants to stress that the return value could be a
> number of positive or negative values, we just use for the common case
> (0: success, -1: error):
>
>         if (!gethostname(...)) {
>
> > +             if ((strlen(hostname) + strlen(hostname_prefix)) > HOST_NAME_MAX) {
> > +                     hostname[strlen(hostname)-strlen(hostname_prefix)] = '\0';
> > +             }
>
> For consistency: no curly brackets for statements that fit a single
> line.
>
> > +             sprintf(pasta_hostname, "%s%s", hostname_prefix, hostname);
>
> You could drop all this if you initialise the target variable directly, say:
>
>         char hostname[HOST_NAME_MAX + 1] = "pasta-";
>
> then you can gethostname() on it + sizeof("pasta-") - 1 (NULL
> terminating byte returned by sizeof()), directly.
>
> Using a #define for "pasta-":
>
>         gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
>                     HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX));
>
> > +
> > +             if (sethostname(pasta_hostname, strlen(pasta_hostname)) != 0) {
>
> Same here about !sethostname() vs. sethostname() != 0, and curly
> brackets.
>
> > +                     warn("Unable to set pasta-prefixed hostname");
> > +             }
> > +     }
> > +
> >       /* Wait for the parent to be ready: see main() */
> >       sigemptyset(&set);
> >       sigaddset(&set, SIGUSR1);
>
> --
> Stefano
>

Re: [PATCH] pasta.c: modify hostname when detaching new namespace

[Stefano Brivio]

Danish, it would be easier if you answered inline. If Gmail is making
it hard, perhaps switch to an email client (I use claws-mail)? Anyway,
it's not a big issue for the moment:

On Thu, 23 May 2024 19:22:16 +0530
Danish Prakash <contact@danishpraka.sh> wrote:

> Thanks for the review and the links.
> 
> > +       if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX))) {
> > +               if (sethostname(hostname, strlen(hostname)))
> > +                       debug("Unable to set pasta-prefixed hostname");
> >         }  
> 
> The above snippet, although it looks correct,

Wait, I didn't suggest if (!gethostname(...)), I suggested
gethostname(...). The ! there is yours. :)

> doesn't work in cases where the hostname is long enough (~>58 chars).
> It works fine for shorter hostnames.

In any case, it depends on how you define "doesn't work". What should
we do if the original hostname is long enough that we can't prefix
"pasta-" while fitting in 63 characters?

Append it anyway and truncate the original hostname (what my line did),
or leave it like it is (what your snippet does, I guess)? It's a matter
of taste I'd say.

> The call to `gethostname()` sets errno to 36
> (ENAMETOOLONG). Upon looking further, it seems that even though the
> manpage for `gethostname(char *name, size_t len)` states..
> 
> >  If the null-terminated hostname is too large to fit, then the name is truncated, and no error is returned (but see NOTES below).

...then the NOTES section disappeared and this ended up in "ERRORS", in
the Linux man-pages:

       ENAMETOOLONG
              (glibc gethostname()) len is smaller than the actual size.
              (Before glibc 2.1, glibc uses EINVAL for this case.)

> ...It would still throw ENAMETOOLONG if the value of len is smaller
> than `strlen(hostname)+1` (referring to the snippet below). I'm not
> sure if I'm missing out on an edge case while calculating the
> boundaries here because the `memcpy` call is certainly truncating the
> hostname[1] before ending up returning ENAMETOOLONG, which seems
> conflicting[1]:

...no, I don't think you're missing out on any edge case, you simply
missed that part of the man page. Note that we need to play nicely with
other C libraries too (especially musl), so error or not, we should do
the right/same thing.

Perhaps most robust approach:

	if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
			 HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
	    errno == ENAMETOOLONG) {

so that if it's glibc, and it truncates, we'll just go ahead with our
truncated name, but not if there's any other error.

Note that you don't strictly need the NULL byte at the end, see
sethostname(2) -- just make sure you don't print it or anything like
that.

-- 
Stefano

Re: [PATCH] pasta.c: modify hostname when detaching new namespace

[Danish Prakash]

On 5/23/24 19:53, Stefano Brivio wrote:
> Danish, it would be easier if you answered inline. If Gmail is making
> it hard, perhaps switch to an email client (I use claws-mail)? Anyway,
> it's not a big issue for the moment:

Yeah, something went wrong with that last one, sorry about that. I 
switched to a new email and didn't properly set it up. Hopefully it'll 
be better now.

> On Thu, 23 May 2024 19:22:16 +0530
> Danish Prakash <contact@danishpraka.sh> wrote:
> 
>> Thanks for the review and the links.
>>
>>> +       if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX))) {
>>> +               if (sethostname(hostname, strlen(hostname)))
>>> +                       debug("Unable to set pasta-prefixed hostname");
>>>          }
>>
>> The above snippet, although it looks correct,
> 
> Wait, I didn't suggest if (!gethostname(...)), I suggested
> gethostname(...). The ! there is yours. :)

I misread your point earlier i guess. My intent here is to change the 
hostname (sethostname) if gethostname succeeds.

>> doesn't work in cases where the hostname is long enough (~>58 chars).
>> It works fine for shorter hostnames.
> 
> In any case, it depends on how you define "doesn't work". What should
> we do if the original hostname is long enough that we can't prefix
> "pasta-" while fitting in 63 characters?
> 
> Append it anyway and truncate the original hostname (what my line did),
> or leave it like it is (what your snippet does, I guess)? It's a matter
> of taste I'd say.

I'm not sure I follow this part here, in your "line", are you getting 
and setting the hostname in two different conditionals? Because both 
would be doing the same thing ie. truncating the hostname but given how 
gethostname is implemented, the call would fail if len(hostname) > len 
passed to gethostname...

> ...no, I don't think you're missing out on any edge case, you simply
> missed that part of the man page. Note that we need to play nicely with
> other C libraries too (especially musl), so error or not, we should do
> the right/same thing.
> 
> Perhaps most robust approach:
> 
> 	if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> 			 HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> 	    errno == ENAMETOOLONG) {
> 
> so that if it's glibc, and it truncates, we'll just go ahead with our
> truncated name, but not if there's any other error.

...But this seems to do the job because gethostname returns the 
truncated hostname *along* with ENAMETOOLONG in the edge case where 
hostname is longer than the provided len, so as long as we're handling 
and are okay with that error, we get the desired result.

I'll send along the updated patch shortly.

-- 
danishpraka.sh

[PATCH] pasta.c: modify hostname when detaching new namespace

[Danish Prakash]

When invoking pasta without any arguments, it's difficult
to tell whether we are in the new namespace or not leaving
users a bit confused. This change modifies the host namespace
to add a prefix "pasta-" to make it a bit more obvious.

Signed-off-by: Danish Prakash <contact@danishpraka.sh>
---
 pasta.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/pasta.c b/pasta.c
index 31e1e00..90afd74 100644
--- a/pasta.c
+++ b/pasta.c
@@ -50,6 +50,8 @@
 #include "netlink.h"
 #include "log.h"
 
+#define HOSTNAME_PREFIX		"pasta-"
+
 /* PID of child, in case we created a namespace */
 int pasta_child_pid;
 
@@ -178,6 +180,7 @@ struct pasta_spawn_cmd_arg {
  */
 static int pasta_spawn_cmd(void *arg)
 {
+	char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
 	const struct pasta_spawn_cmd_arg *a;
 	sigset_t set;
 
@@ -188,6 +191,12 @@ static int pasta_spawn_cmd(void *arg)
 	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
 		warn("Cannot set ping_group_range, ICMP requests might fail");
 
+	if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
+            errno == ENAMETOOLONG ) {
+		if (sethostname(hostname, strlen(hostname)))
+			warn("Unable to set pasta-prefixed hostname");
+	}
+
 	/* Wait for the parent to be ready: see main() */
 	sigemptyset(&set);
 	sigaddset(&set, SIGUSR1);
-- 
@@ -50,6 +50,8 @@
 #include "netlink.h"
 #include "log.h"
 
+#define HOSTNAME_PREFIX		"pasta-"
+
 /* PID of child, in case we created a namespace */
 int pasta_child_pid;
 
@@ -178,6 +180,7 @@ struct pasta_spawn_cmd_arg {
  */
 static int pasta_spawn_cmd(void *arg)
 {
+	char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
 	const struct pasta_spawn_cmd_arg *a;
 	sigset_t set;
 
@@ -188,6 +191,12 @@ static int pasta_spawn_cmd(void *arg)
 	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
 		warn("Cannot set ping_group_range, ICMP requests might fail");
 
+	if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
+            errno == ENAMETOOLONG ) {
+		if (sethostname(hostname, strlen(hostname)))
+			warn("Unable to set pasta-prefixed hostname");
+	}
+
 	/* Wait for the parent to be ready: see main() */
 	sigemptyset(&set);
 	sigaddset(&set, SIGUSR1);
-- 
2.45.1

Re: [PATCH] pasta.c: modify hostname when detaching new namespace

[Stefano Brivio]

On Fri, 24 May 2024 18:15:14 +0530
Danish Prakash <contact@danishpraka.sh> wrote:

> On 5/23/24 19:53, Stefano Brivio wrote:
> > Danish, it would be easier if you answered inline. If Gmail is making
> > it hard, perhaps switch to an email client (I use claws-mail)? Anyway,
> > it's not a big issue for the moment:  
> 
> Yeah, something went wrong with that last one, sorry about that. I 
> switched to a new email and didn't properly set it up. Hopefully it'll 
> be better now.
> 
> > On Thu, 23 May 2024 19:22:16 +0530
> > Danish Prakash <contact@danishpraka.sh> wrote:
> >   
> >> Thanks for the review and the links.
> >>  
> >>> +       if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX))) {
> >>> +               if (sethostname(hostname, strlen(hostname)))
> >>> +                       debug("Unable to set pasta-prefixed hostname");
> >>>          }  
> >>
> >> The above snippet, although it looks correct,  
> > 
> > Wait, I didn't suggest if (!gethostname(...)), I suggested
> > gethostname(...). The ! there is yours. :)  
> 
> I misread your point earlier i guess. My intent here is to change the 
> hostname (sethostname) if gethostname succeeds.

Sure, I understand, I just suggested a particular version of the
gethostname() call. I didn't comment on what's around it.

> >> doesn't work in cases where the hostname is long enough (~>58 chars).
> >> It works fine for shorter hostnames.  
> > 
> > In any case, it depends on how you define "doesn't work". What should
> > we do if the original hostname is long enough that we can't prefix
> > "pasta-" while fitting in 63 characters?
> > 
> > Append it anyway and truncate the original hostname (what my line did),
> > or leave it like it is (what your snippet does, I guess)? It's a matter
> > of taste I'd say.  
> 
> I'm not sure I follow this part here, in your "line", are you getting 
> and setting the hostname in two different conditionals?

Not really, I wasn't commenting on sethostname() at all, but in your
snippet you quoted above, yes, and I think it's fine, because there
might be other reasons why sethostname() fails.

> Because both 
> would be doing the same thing ie. truncating the hostname but given how 
> gethostname is implemented, the call would fail if len(hostname) > len 
> passed to gethostname...
> 
> > ...no, I don't think you're missing out on any edge case, you simply
> > missed that part of the man page. Note that we need to play nicely with
> > other C libraries too (especially musl), so error or not, we should do
> > the right/same thing.
> > 
> > Perhaps most robust approach:
> > 
> > 	if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> > 			 HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> > 	    errno == ENAMETOOLONG) {
> > 
> > so that if it's glibc, and it truncates, we'll just go ahead with our
> > truncated name, but not if there's any other error.  
> 
> ...But this seems to do the job because gethostname returns the 
> truncated hostname *along* with ENAMETOOLONG in the edge case where 
> hostname is longer than the provided len, so as long as we're handling 
> and are okay with that error, we get the desired result.
> 
> I'll send along the updated patch shortly.
> 

-- 
Stefano

Re: [PATCH] pasta.c: modify hostname when detaching new namespace

[Stefano Brivio]

On Fri, 24 May 2024 18:18:53 +0530
Danish Prakash <contact@danishpraka.sh> wrote:

> When invoking pasta without any arguments, it's difficult
> to tell whether we are in the new namespace or not leaving
> users a bit confused. This change modifies the host namespace
> to add a prefix "pasta-" to make it a bit more obvious.
> 
> Signed-off-by: Danish Prakash <contact@danishpraka.sh>
> ---
>  pasta.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/pasta.c b/pasta.c
> index 31e1e00..90afd74 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -50,6 +50,8 @@
>  #include "netlink.h"
>  #include "log.h"
>  
> +#define HOSTNAME_PREFIX		"pasta-"
> +
>  /* PID of child, in case we created a namespace */
>  int pasta_child_pid;
>  
> @@ -178,6 +180,7 @@ struct pasta_spawn_cmd_arg {
>   */
>  static int pasta_spawn_cmd(void *arg)
>  {
> +	char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
>  	const struct pasta_spawn_cmd_arg *a;
>  	sigset_t set;
>  
> @@ -188,6 +191,12 @@ static int pasta_spawn_cmd(void *arg)
>  	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
>  		warn("Cannot set ping_group_range, ICMP requests might fail");
>  
> +	if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||

Following the Linux kernel coding style also means we try to stick into
80 columns where possible:

  https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings

...so there was a reason why I proposed this line like I did, with the
line splits. These subtleties, I can also fix them up on merge.

> +            errno == ENAMETOOLONG ) {

Excess whitespace between ENAMETOOLONG and ). Same here, I would fix
this up on merge.

> +		if (sethostname(hostname, strlen(hostname)))

So, I mentioned before that you don't really need to set a NULL
terminating byte for sethostname() itself, because it takes a length.

But strlen() needs it. If gethostname() truncated the hostname,
according to POSIX, it's unspecified whether we'll have a NULL byte at
the end of 'hostname', and strlen() would read out-of-bounds, past the
end of 'hostname'.

That's not an issue with glibc, but if POSIX says it's not guaranteed,
we shouldn't take anything for granted.

I would suggest that you simply add a NULL byte at HOST_NAME_MAX,
unconditionally, that should cover the normal case as well as the
ENAMETOOLONG case. I haven't tested this by the way.

> +			warn("Unable to set pasta-prefixed hostname");
> +	}
> +
>  	/* Wait for the parent to be ready: see main() */
>  	sigemptyset(&set);
>  	sigaddset(&set, SIGUSR1);

-- 
Stefano

Re: [PATCH] pasta.c: modify hostname when detaching new namespace

[Stefano Brivio]

Hi Danish,

On Fri, 24 May 2024 19:39:18 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Fri, 24 May 2024 18:18:53 +0530
> Danish Prakash <contact@danishpraka.sh> wrote:
> 
> > When invoking pasta without any arguments, it's difficult
> > to tell whether we are in the new namespace or not leaving
> > users a bit confused. This change modifies the host namespace
> > to add a prefix "pasta-" to make it a bit more obvious.
> > 
> > Signed-off-by: Danish Prakash <contact@danishpraka.sh>
> > ---
> >  pasta.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/pasta.c b/pasta.c
> > index 31e1e00..90afd74 100644
> > --- a/pasta.c
> > +++ b/pasta.c
> > @@ -50,6 +50,8 @@
> >  #include "netlink.h"
> >  #include "log.h"
> >  
> > +#define HOSTNAME_PREFIX		"pasta-"
> > +
> >  /* PID of child, in case we created a namespace */
> >  int pasta_child_pid;
> >  
> > @@ -178,6 +180,7 @@ struct pasta_spawn_cmd_arg {
> >   */
> >  static int pasta_spawn_cmd(void *arg)
> >  {
> > +	char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
> >  	const struct pasta_spawn_cmd_arg *a;
> >  	sigset_t set;
> >  
> > @@ -188,6 +191,12 @@ static int pasta_spawn_cmd(void *arg)
> >  	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
> >  		warn("Cannot set ping_group_range, ICMP requests might fail");
> >  
> > +	if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1, HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||  
> 
> Following the Linux kernel coding style also means we try to stick into
> 80 columns where possible:
> 
>   https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
> 
> ...so there was a reason why I proposed this line like I did, with the
> line splits. These subtleties, I can also fix them up on merge.
> 
> > +            errno == ENAMETOOLONG ) {  
> 
> Excess whitespace between ENAMETOOLONG and ). Same here, I would fix
> this up on merge.
> 
> > +		if (sethostname(hostname, strlen(hostname)))  
> 
> So, I mentioned before that you don't really need to set a NULL
> terminating byte for sethostname() itself, because it takes a length.
> 
> But strlen() needs it. If gethostname() truncated the hostname,
> according to POSIX, it's unspecified whether we'll have a NULL byte at
> the end of 'hostname', and strlen() would read out-of-bounds, past the
> end of 'hostname'.
> 
> That's not an issue with glibc, but if POSIX says it's not guaranteed,
> we shouldn't take anything for granted.
> 
> I would suggest that you simply add a NULL byte at HOST_NAME_MAX,
> unconditionally, that should cover the normal case as well as the
> ENAMETOOLONG case. I haven't tested this by the way.
> 
> > +			warn("Unable to set pasta-prefixed hostname");
> > +	}
> > +
> >  	/* Wait for the parent to be ready: see main() */
> >  	sigemptyset(&set);
> >  	sigaddset(&set, SIGUSR1);  
> 

I didn't get any follow up on this patch. Did you actually mean to
abandon it?

If you prefer, I can also pick it up from there.

-- 
Stefano

Re: [PATCH] pasta.c: modify hostname when detaching new namespace

[Danish Prakash]

Hi Stefano,

I was caught up with some work last month, hence the delay in responses
since my last email.

On 5/24/24 11:09 PM, Stefano Brivio wrote:
>> +		if (sethostname(hostname, strlen(hostname)))
> 
> So, I mentioned before that you don't really need to set a NULL
> terminating byte for sethostname() itself, because it takes a length.
> 
> But strlen() needs it. If gethostname() truncated the hostname,
> according to POSIX, it's unspecified whether we'll have a NULL byte at
> the end of 'hostname', and strlen() would read out-of-bounds, past the
> end of 'hostname'.
> 
> That's not an issue with glibc, but if POSIX says it's not guaranteed,
> we shouldn't take anything for granted.
> 
> I would suggest that you simply add a NULL byte at HOST_NAME_MAX,
> unconditionally, that should cover the normal case as well as the
> ENAMETOOLONG case. I haven't tested this by the way.

Did you mean explicitly setting the NULL byte to `hostname`?

        hostname[HOST_NAME_MAX] = '\0';

Doing that after gethostname() and before sethostname() yields the
desired result. I tested a few cases, seems to be fine.

> 
>> +			warn("Unable to set pasta-prefixed hostname");
>> +	}
>> +
>>  	/* Wait for the parent to be ready: see main() */
>>  	sigemptyset(&set);
>>  	sigaddset(&set, SIGUSR1);
> 

-- 
danishpraka.sh

Re: [PATCH] pasta.c: modify hostname when detaching new namespace

[Stefano Brivio]

On Mon, 29 Jul 2024 19:26:28 +0530
Danish Prakash <contact@danishpraka.sh> wrote:

> Hi Stefano,
> 
> I was caught up with some work last month, hence the delay in responses
> since my last email.
> 
> On 5/24/24 11:09 PM, Stefano Brivio wrote:
> >> +		if (sethostname(hostname, strlen(hostname)))  
> > 
> > So, I mentioned before that you don't really need to set a NULL
> > terminating byte for sethostname() itself, because it takes a length.
> > 
> > But strlen() needs it. If gethostname() truncated the hostname,
> > according to POSIX, it's unspecified whether we'll have a NULL byte at
> > the end of 'hostname', and strlen() would read out-of-bounds, past the
> > end of 'hostname'.
> > 
> > That's not an issue with glibc, but if POSIX says it's not guaranteed,
> > we shouldn't take anything for granted.
> > 
> > I would suggest that you simply add a NULL byte at HOST_NAME_MAX,
> > unconditionally, that should cover the normal case as well as the
> > ENAMETOOLONG case. I haven't tested this by the way.  
> 
> Did you mean explicitly setting the NULL byte to `hostname`?
> 
>         hostname[HOST_NAME_MAX] = '\0';

Yes, exactly.

> Doing that after gethostname() and before sethostname() yields the
> desired result. I tested a few cases, seems to be fine.

Yes, I don't expect any difference with glibc, but better safe than
sorry (with other C libraries). Thanks.

-- 
Stefano