| Message ID | 20200803205529.2232-1-ericonr@disroot.org |
|---|---|
| State | New |
| Series | "extend gethostid beyond a stub" |
| Headers | show |
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; }
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
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.
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(-)