[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20200716170737.GA21160@openwall.com>
Date: Thu, 16 Jul 2020 19:07:37 +0200
From: Solar Designer <solar@...nwall.com>
To: owl-dev@...ts.openwall.com
Subject: Re: [PATCH 0/5] pam_tcb update
On Thu, Jul 16, 2020 at 07:49:56PM +0300, Dmitry V. Levin wrote:
> On Thu, Jul 16, 2020 at 06:30:22PM +0200, Solar Designer wrote:
> > On Thu, Jul 16, 2020 at 12:56:07AM +0300, Dmitry V. Levin wrote:
> > > These -Wpointer-sign warnings look harmless.
> > > I suppose they are quite old, though.
> > > I've committed a fix.
> >
> > The warnings are gone for me. Thanks!
> >
> > > I also see the following warnings:
> > > nss.c:111:3: warning: 'readdir_r' is deprecated [-Wdeprecated-declarations]
> > > tcb_unconvert.c:245:2: warning: ignoring return value of 'chown', declared with attribute warn_unused_result [-Wunused-result]
> > > tcb_chkpwd.c:58:4: warning: ignoring return value of 'seteuid', declared with attribute warn_unused_result [-Wunused-result]
> > > tcb_chkpwd.c:61:4: warning: ignoring return value of 'seteuid', declared with attribute warn_unused_result [-Wunused-result]
> > >
> > > We probably should use readdir instead of readdir_r, this would fix the
> > > first warning.
> >
> > Could we? This is inside _nss_tcb_getspent_r(), which is sort of meant
> > to be reentrant - but I don't understand what this really means for that
> > particular function given that it's stateful - each subsequent call is
> > supposed to return the next entry. Maybe what is meant is only that the
> > return value buffer is caller-provided, so if different buffers are used
> > then the previous ones are not overwritten by subsequent calls. Not
> > that the function is really reentrant. Do you know?
> >
> > Also, why is readdir_r() deprecated? Is readdir() now reentrant?
>
> The current edition of readdir_r(3) manpage at
> https://man7.org/linux/man-pages/man3/readdir_r.3.html says:
> "It is recommended that applications use readdir(3) instead of readdir_r()."
> It also gives a rationale for this move.
OK, read the rationale. It looks unsafe for us to simply switch to
readdir(), because in our case concurrent calls to _nss_tcb_getspent_r()
would most likely result in reads from the same directory stream, which
is still described as unsafe for readdir(). And if we don't support
concurrent calls, then why do we have _r in _nss_tcb_getspent_r()?
First and foremost, we need to understand what properties our
_nss_tcb_getspent_r() is supposed to have, and only then decide on how
to provide those best.
> > > There is not much we could do about -Wunused-result warnings.
> > > In case of 'chown' it make sense to issue a warning.
> >
> > Yes, I think we can call perror() just like we do on the previous
> > chown()'s error, but we don't have to return 1 on this one.
>
> OK
>
> > > In case of first 'seteuid' error we could skip the second 'seteuid'
> > > invocation.
> >
> > Then what to do if the first one succeeds, but the second one fails?
> > Luckily, this is a separate program, which at that point is about to
> > terminate. Also, this whole piece is for NIS (right?), the support for
> > which we've just dropped from the rest of tcb. Should we possibly drop
> > this piece, too?
>
> Yes, this is the only remaining piece of NIS+ support, so we can safely
> drop it instead of trying to come up with a fix.
Please implement these changes (chown() return value check, and drop the
remaining NIS+ support piece).
Thanks,
Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
