findutils-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Findutils-patches] [PATCH] find: handle more readdir(3) errors


From: Bernhard Voelker
Subject: Re: [Findutils-patches] [PATCH] find: handle more readdir(3) errors
Date: Tue, 1 Nov 2016 22:32:19 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/01/2016 09:06 PM, Eric Blake wrote:
> On 11/01/2016 02:37 PM, Bernhard Voelker wrote:
>> Similar to the FTS readdir fix in v4.6.0-72-g155c9d1, handle the last two 
>> unhandled readdir(3) errors.
>> 
>> * find/pred.c (pred_empty): Do the above. * lib/fdleak.c (get_proc_max_fd): 
>> Likewise.  While at it, fix the 
>> condition to only skip "." and ".."; previously, also other files beginning 
>> with ".." would have been skipped -
>> that was theoretically, as we only expect the FD files in "/proc/self/fd". 
>> --- find/pred.c  |  8 ++++++++ 
>> lib/fdleak.c | 21 +++++++++++++++++---- 2 files changed, 25 insertions(+), 4 
>> deletions(-)
>> 
>> diff --git a/find/pred.c b/find/pred.c index f7e9b59..93f82b6 100644 --- 
>> a/find/pred.c +++ b/find/pred.c
> 
> [expanding context]
> 
>> @@ -380,6 +380,7 @@ pred_empty (const char *pathname, struct stat *stat_buf, 
>> struct predicate *pred_ 
>> state.exit_status = 1; return false; } +      errno = 0; for (dp = readdir 
>> (d); dp; dp = readdir (d))
> 
> Generally wrong; you have to clear errno before EVERY call to readdir(), not 
> just the first.  Either with
> 'for(..;..; errno = 0, dp = readdir (d))', or by...
> 
>> { if (dp->d_name[0] != '.'
> || (dp->d_name[1] != '\0' && (dp->d_name[1] != '.' || dp->d_name[2] != 
> '\0'))) { empty = false;
>> break; } }
> 
> ...clearing errno at the end of the loop.  But for _this particular loop_, 
> the body does not touch errno, which
> means later iterations are left with it still 0 from the earlier iterations,
> 
>> +      if (errno) +  { +       /* Handle errors from readdir(3). */ +        
>>   error (0, errno, "%s",
>> safely_quote_err_filename (0, pathname)); +    state.exit_status = 1; +      
>>   return false; +       }
> 
> so I can agree to this part.

I knew that it's maybe a bit risky if someone adds some errno-clobbering
code in future, so I should at least have added a comment for this.

> However,
> 
>> +++ b/lib/fdleak.c
> 
>> -      while ((dent=readdir (dir)) != NULL) -        { +      while (1) +    
>>     { +   errno = 0;

__________^^^^^^^^^^

>> +      dent = readdir (dir); +         if (NULL == dent) +       { +       
>> if (errno) +              { +               error (0, errno, "%s",
>> quotearg_n_style (0, locale_quoting_style, path)); +           good = 0; +   
>>         } +           break; +      } + if
>> (dent->d_name[0] != '.' -          || (dent->d_name[0] != 0 -                
>>   && dent->d_name[1] != 0 && dent->d_name[1] !=
>> '.')) +            || (dent->d_name[1] != 0 +                  && 
>> (dent->d_name[1] != '.' || dent->d_name[2] != 0))) { const int fd
>> = safe_atoi (dent->d_name, literal_quoting_style);
> 
> THIS loop is a demonstration of how not to use readdir; safe_atoi() can 
> clobber errno, so you are no longer
> guaranteed that each loop iteration is starting with errno == 0.  You'll have 
> to tweak the patch.
> 

errno _is_ initialized right before each readdir() call, see above,
so I don't see the problem here.

Thanks for the review.

Have a nice day,
Berny
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBAgAGBQJYGQnjAAoJEEZQLveWkXGVt3sH/jiO87av9l5VnV3mwAI0nXHg
aWMtXKOZ4q9pIp0qm+qpm9QPoca5F3AptqwYXzD1D5ti+AJSKSMUcCcvaFH31Qyp
ZsWSU0IqxiA3mCzZ9Z2KSz5BfLPtVJNtL6Ql1uqfGwHvo+EuQtZZ1nDZhtxfmyuc
YQ/AVFksmL0b29uhx57EqUQo6FzQZ4AQ58FDsFE3+xgrrKwQO0xxnDXY0b/HDY3O
dNhbv57zG3xvxG3Yh/xeWrybd5/AQrr0EKj+9skqw2O1lKenTMTtMSmQm+Zaj2rF
/iTcfHBbMwcjIxPNSmf/EElhqHrBotQlsytYqKHUQh2OEda8IgRaGNA0y79GacY=
=+3Fa
-----END PGP SIGNATURE-----



reply via email to

[Prev in Thread] Current Thread [Next in Thread]