Re: [hackers] [sbase][PATCH] ls: abort a directory if we cannot opendir it
On Tue, Aug 22, 2017 at 04:51:37PM +1200, David Phillips wrote:
> 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.
> ---
> 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
> @@ -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);
> --
> 2.14.1
>
Applied, thanks.
--
Kind regards,
Hiltjo
signature.asc
Description: PGP signature
Re: [hackers] [sbase][PATCH] ls: abort a directory if we cannot opendir it
> 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. Ah, right! > 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. No problem, I should have put the patch code in context too. Looks good to me, thanks!
Re: [hackers] [sbase][PATCH] ls: abort a directory if we cannot opendir it
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
> > @@ -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);
>
Re: [hackers] [sbase][PATCH] ls: abort a directory if we cannot opendir it
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
> @@ -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);
