extend gethostid beyond a stub

Submitted by Érico Rolim on Aug. 3, 2020, 8:55 p.m.

Details

Message ID 20200803205529.2232-1-ericonr@disroot.org
State New
Series "extend gethostid beyond a stub"
Headers show

Commit Message

Érico Rolim Aug. 3, 2020, 8:55 p.m.
From: Érico Rolim <erico.erc@gmail.com>

Implement part of the glibc behavior, where the 32-bit identifier stored
in /etc/hostid, if the file exists, is returned. If this file doesn't
contain at least 32 bits or can't be opened for some reason, return 0.
---
 src/misc/gethostid.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/misc/gethostid.c b/src/misc/gethostid.c
index 25bb35db..e2e98b99 100644
--- a/src/misc/gethostid.c
+++ b/src/misc/gethostid.c
@@ -1,6 +1,19 @@ 
 #include <unistd.h>
+#include <stdio.h>
 
 long gethostid()
 {
-	return 0;
+	FILE *f;
+	unsigned char hostid[4];
+	long rv = 0;
+
+	f = fopen("/etc/hostid", "reb");
+	if (f) {
+		if (fread(hostid, 1, 4, f) == 4) {
+			rv = (hostid[3] << 24) | (hostid[2] << 16) | (hostid[1] << 8) | hostid[0];
+		}
+		fclose(f);
+	}
+
+	return rv;
 }

Comments

Rich Felker Aug. 4, 2020, 3:02 a.m.
On Mon, Aug 03, 2020 at 05:55:29PM -0300, Érico Rolim wrote:
> From: Érico Rolim <erico.erc@gmail.com>
> 
> Implement part of the glibc behavior, where the 32-bit identifier stored
> in /etc/hostid, if the file exists, is returned. If this file doesn't
> contain at least 32 bits or can't be opened for some reason, return 0.
> ---
>  src/misc/gethostid.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/misc/gethostid.c b/src/misc/gethostid.c
> index 25bb35db..e2e98b99 100644
> --- a/src/misc/gethostid.c
> +++ b/src/misc/gethostid.c
> @@ -1,6 +1,19 @@
>  #include <unistd.h>
> +#include <stdio.h>
>  
>  long gethostid()
>  {
> -	return 0;
> +	FILE *f;
> +	unsigned char hostid[4];
> +	long rv = 0;
> +
> +	f = fopen("/etc/hostid", "reb");
> +	if (f) {
> +		if (fread(hostid, 1, 4, f) == 4) {
> +			rv = (hostid[3] << 24) | (hostid[2] << 16) | (hostid[1] << 8) | hostid[0];
> +		}
> +		fclose(f);
> +	}
> +
> +	return rv;
>  }
> -- 
> 2.28.0

I somewhat dislike the use of stdio here, but this is something of a
junk function that's not really worth writing read() retry loop, etc.

hostid[3]<<24 is UB due to integer overflow (the promoted type is int,
a signed type). This could be fixed via promotion to unsigned before
the shift, but rather than constructing the value manually like this
I'd probably lean towards reading it into a uint32_t object x then
returning ntohl(x).

It's unfortunate that fopen can fail for spurious reasons like ENOMEM
or EMFILE/ENFILE, and that gethostid has no way of reporting this
error rather than returning the wrong id, but this seems like a
fundamental design bug in the interface and not something we can fix,
at least not while using the existing design with data in a file. I
think it could be avoided by using readlink() and storing the id in
the contents of a symlink, which should have no spurious failure
modes, but I'm not really keen on inventing a new convention for this
fundamentally-broken function.

So overall this looks pretty good. I'll revisit it after release and
see if anyone else has thoughts on it in the mean time.

Rich
Andre McCurdy Aug. 4, 2020, 7:02 a.m.
On Mon, Aug 3, 2020 at 8:02 PM Rich Felker <dalias@libc.org> wrote:
>
> On Mon, Aug 03, 2020 at 05:55:29PM -0300, Érico Rolim wrote:
> > From: Érico Rolim <erico.erc@gmail.com>
> >
> > Implement part of the glibc behavior, where the 32-bit identifier stored
> > in /etc/hostid, if the file exists, is returned. If this file doesn't
> > contain at least 32 bits or can't be opened for some reason, return 0.
> > ---
> >  src/misc/gethostid.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/misc/gethostid.c b/src/misc/gethostid.c
> > index 25bb35db..e2e98b99 100644
> > --- a/src/misc/gethostid.c
> > +++ b/src/misc/gethostid.c
> > @@ -1,6 +1,19 @@
> >  #include <unistd.h>
> > +#include <stdio.h>
> >
> >  long gethostid()
> >  {
> > -     return 0;
> > +     FILE *f;
> > +     unsigned char hostid[4];
> > +     long rv = 0;
> > +
> > +     f = fopen("/etc/hostid", "reb");
> > +     if (f) {
> > +             if (fread(hostid, 1, 4, f) == 4) {
> > +                     rv = (hostid[3] << 24) | (hostid[2] << 16) | (hostid[1] << 8) | hostid[0];
> > +             }
> > +             fclose(f);
> > +     }
> > +
> > +     return rv;
> >  }
> > --
> > 2.28.0
>
> I somewhat dislike the use of stdio here, but this is something of a
> junk function that's not really worth writing read() retry loop, etc.
>
> hostid[3]<<24 is UB due to integer overflow (the promoted type is int,
> a signed type). This could be fixed via promotion to unsigned before
> the shift, but rather than constructing the value manually like this
> I'd probably lean towards reading it into a uint32_t object x then
> returning ntohl(x).

The glibc implementation appears to read and write directly into an
int32_t variable, without any endianness conversion. To be
interoperable with /etc/hostid files created by glibc shouldn't musl
skip the ntohl() and just return x ?

> It's unfortunate that fopen can fail for spurious reasons like ENOMEM
> or EMFILE/ENFILE, and that gethostid has no way of reporting this
> error rather than returning the wrong id, but this seems like a
> fundamental design bug in the interface and not something we can fix,
> at least not while using the existing design with data in a file. I
> think it could be avoided by using readlink() and storing the id in
> the contents of a symlink, which should have no spurious failure
> modes, but I'm not really keen on inventing a new convention for this
> fundamentally-broken function.
>
> So overall this looks pretty good. I'll revisit it after release and
> see if anyone else has thoughts on it in the mean time.