[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200716164955.GA6167@altlinux.org>
Date: Thu, 16 Jul 2020 19:49:56 +0300
From: "Dmitry V. Levin" <ldv@...linux.org>
To: owl-dev@...ts.openwall.com
Subject: Re: [PATCH 0/5] pam_tcb update
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.
> > 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.
--
ldv
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.
