[patch] Add error check for fchmod in ldconfig.c

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] Add error check for fchmod in ldconfig.c

Nan Xiao-2
Hi tech@,

Maybe a error check of fchmod is necessary. Sorry if I am wrong, thanks!

Index: ldconfig.c
===================================================================
RCS file: /cvs/src/libexec/ld.so/ldconfig/ldconfig.c,v
retrieving revision 1.37
diff -u -p -r1.37 ldconfig.c
--- ldconfig.c 26 Apr 2018 12:42:50 -0000 1.37
+++ ldconfig.c 7 Jun 2018 07:04:45 -0000
@@ -386,7 +386,11 @@ buildhints(void)
  warn("%s", tmpfilenam);
  goto out;
  }
- fchmod(fd, 0444);
+
+ if (fchmod(fd, 0444) == -1) {
+ warn("%s", tmpfilenam);
+ goto out;
+ }

  if (write(fd, &hdr, sizeof(struct hints_header)) !=
     sizeof(struct hints_header)) {

--
Best Regards
Nan Xiao

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Add error check for fchmod in ldconfig.c

Scott Cheloha
On Thu, Jun 07, 2018 at 03:31:16PM +0800, Nan Xiao wrote:
> Hi tech@,
>
> Maybe a error check of fchmod is necessary. [...]

In that spot I think the only way fchmod can fail is with EIO.

But, for sake of discussion, suppose it fails with EIO and then all
the subsequent writes succeed and the EIO, for whatever reason, was
spurious or somehow not indicative of deeper instability, and the
system remains up and stable afterward: I think there are then two
problems with proceeding after fchmod fails.

One, installing ld.so.hints when it's writable by root sounds like
a bad idea.  I'm not sure it matters in practice as root can open a
read-only file with write privileges anyway, but in principle the hints
file should be read-only for everyone.

Two, installing ld.so.hints when it isn't readable by the rest of the
system sort of defeats the purpose of ld.so.hints: speeding up shared
library lookup for everyone.

Unless this is the very first run of ldconfig, there will be a legible
and useful hints file in-place already... which we'd then overwrite
with one only readable by root processes.

Ideally, most processes are not root processes, so this potentially adds
some overhead for all such processes without LD_LIBRARY_PATH set, which
would not be ideal.

--

This patch is ok cheloha@ and I can commit this if someone else is ok with
it, but I'd really like to hear from someone with more familiarity with
the run-time linking process: is my analysis here off-base?

--
Scott Cheloha

> Index: ldconfig.c
> ===================================================================
> RCS file: /cvs/src/libexec/ld.so/ldconfig/ldconfig.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 ldconfig.c
> --- ldconfig.c 26 Apr 2018 12:42:50 -0000 1.37
> +++ ldconfig.c 7 Jun 2018 07:04:45 -0000
> @@ -386,7 +386,11 @@ buildhints(void)
>   warn("%s", tmpfilenam);
>   goto out;
>   }
> - fchmod(fd, 0444);
> +
> + if (fchmod(fd, 0444) == -1) {
> + warn("%s", tmpfilenam);
> + goto out;
> + }
>
>   if (write(fd, &hdr, sizeof(struct hints_header)) !=
>      sizeof(struct hints_header)) {
>
> --
> Best Regards
> Nan Xiao
>

Reply | Threaded
Open this post in threaded view
|

Re: [patch] Add error check for fchmod in ldconfig.c

Philip Guenther-2
On Thu, Jun 7, 2018 at 7:16 PM, Scott Cheloha <[hidden email]>
wrote:

> On Thu, Jun 07, 2018 at 03:31:16PM +0800, Nan Xiao wrote:
> > Hi tech@,
> >
> > Maybe a error check of fchmod is necessary. [...]
>
...

> This patch is ok cheloha@ and I can commit this if someone else is ok with
> it, but I'd really like to hear from someone with more familiarity with
> the run-time linking process: is my analysis here off-base?


Sounds accurate; ok guenther@