-----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-----
