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(a)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);
--
2.45.1
On Mon, 20 May 2024 14:05:58 +0530 Danish Prakash <contact(a)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(a)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
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/gethos… On Tue, 21 May 2024 at 23:04, Stefano Brivio <sbrivio(a)redhat.com> wrote:On Mon, 20 May 2024 14:05:58 +0530 Danish Prakash <contact(a)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(a)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
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(a)danishpraka.sh> wrote:Thanks for the review and the links.Wait, I didn't suggest if (!gethostname(...)), I suggested gethostname(...). The ! there is yours. :)+ 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.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
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(a)danishpraka.sh> wrote:I misread your point earlier i guess. My intent here is to change the hostname (sethostname) if gethostname succeeds.Thanks for the review and the links.Wait, I didn't suggest if (!gethostname(...)), I suggested gethostname(...). The ! there is yours. :)+ 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,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...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....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
On Fri, 24 May 2024 18:15:14 +0530 Danish Prakash <contact(a)danishpraka.sh> wrote:On 5/23/24 19:53, Stefano Brivio wrote:Sure, I understand, I just suggested a particular version of the gethostname() call. I didn't comment on what's around it.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(a)danishpraka.sh> wrote:I misread your point earlier i guess. My intent here is to change the hostname (sethostname) if gethostname succeeds.Thanks for the review and the links.Wait, I didn't suggest if (!gethostname(...)), I suggested gethostname(...). The ! there is yours. :)+ 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,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.I'm not sure I follow this part here, in your "line", are you getting and setting the hostname in two different conditionals?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.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...-- Stefano...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.
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(a)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);
--
2.45.1
On Fri, 24 May 2024 18:18:53 +0530 Danish Prakash <contact(a)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(a)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-l… ...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
Hi Danish, On Fri, 24 May 2024 19:39:18 +0200 Stefano Brivio <sbrivio(a)redhat.com> wrote:On Fri, 24 May 2024 18:18:53 +0530 Danish Prakash <contact(a)danishpraka.sh> wrote: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. -- StefanoWhen 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(a)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-l… ...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);
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: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.+ 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.-- danishpraka.sh+ warn("Unable to set pasta-prefixed hostname"); + } + /* Wait for the parent to be ready: see main() */ sigemptyset(&set); sigaddset(&set, SIGUSR1);
On Mon, 29 Jul 2024 19:26:28 +0530 Danish Prakash <contact(a)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:Yes, exactly.Did you mean explicitly setting the NULL byte to `hostname`? hostname[HOST_NAME_MAX] = '\0';+ 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.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