Re: [hackers] [sbase][PATCH] ls: abort a directory if we cannot opendir it

From: David Phillips <david_AT_sighup.nz>
Date: Tue, 22 Aug 2017 21:07:26 +1200

Sorry, I should have made the commit message clearer. It is
not the entire operation that is being aborted, but only the
current node, if it cannot be opened.

Before this patch, ls will segfault since it tries to perform
operations on a (null) DIR*. After this patch, it prints the
message, sets the return code and doesn't segfault.

I found this bug while playing with the -R flag (though it is not
isolated to this flag), and I have confirmed that behaviour is
correct as you describe. It will do the regular recursive listing
for all the heirarchy, and in amongst its output is the

        ls: opendir ./foodir: Permission denied

output.

Apologies again if my commit message was a bit ambiguous on what
exactly was being aborted.

All the best,
David

On Tue, Aug 22, 2017 at 10:08:48AM +0200, Quentin Rameau wrote:
> Hello David,
>
> > We should not try and perform operations on an invalid DIR* stream.
> > Instead, we shall let the error message be printed, and the return
> > code set (existing behaviour) and abort afterwards.
>
> Any justification you could provide us with?
>
> AFAIK POSIX specifies the opposite, a diognostic message should be
> issued on stderr and the utility should continue processing the
> remaining of the operands/hierarchy.
>
> > ---
> > ls.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/ls.c b/ls.c
> > index 5080c8f..b716aba 100644
> > --- a/ls.c
> > +++ b/ls.c
> > _AT_@ -250,6 +250,7 @@ lsdir(const char *path, const struct entry *dir)
> > if (!(dp = opendir(dir->name))) {
> > ret = 1;
> > weprintf("opendir %s%s:", path, dir->name);
> > + return;
> > }
> > if (chdir(dir->name) < 0)
> > eprintf("chdir %s:", dir->name);
>
Received on Tue Aug 22 2017 - 11:07:26 CEST