Created attachment 231923 [details] Check for readdir errors Hello, Here is a patch originally reported by Amir Goldstein to one of my projects (fpart), which embeds our version of fts(3). See : https://github.com/martymac/fpart/pull/37/commits/dec9d40d9b4b9551791d61a15442aab8537a26f4 The patch fixes the fact that the fts_build() function does not check for readdir(3) errors and erroneously assumes it always succeeds. The same bug has already been fixed in gnulib, see : https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=6835fc458f30 The attached patch applies to latest -CURRENT. Best regards, Ganael.
This looks ok to me overall. A couple of comments: - We have fts-compat.c and fts-compat11.c which appear to have the same bug. - There are some style bugs: missing parens around the return value in fts_safe_readdir(); extra indentation for line 882 should be by four spaces, not a tab. I also think it'd be better to avoid checking errno directly, and instead modify fts_safe_readdir() to take a pointer to a readdir_errno integer variable, setting that if an error occurs. Maybe it's not worth it to diverge much from other copies of this code, though.
(In reply to Mark Johnston from comment #1) Hello Mark and Ganael, I'm a first-time contributor and I want to fix the bug in fts-compat.c and fts-compat11.c. Should I do it or leave it to Ganael, the original author? If it's OK to fix them, should I include the attachments in this thread or make separate ones?
Hello Mahmoud, Feel free to update the patch if you like, no pb. Just attach your new patch (using the "Add an attachment" link). Also, do not forget to mark it as obsoleting my original patch by checking the "Obsoletes" box. Thanks for your help, Ganael.
Just to add a precision: you should follow Mark's advice and probably re-gen my original patch too, just to be sure we provide the same code for the 3 files. Thanks!
Hello Ganael, I'm sorry this is taking too long. I don't want to post the patches without at least compiling libc to make sure I did not break anything and I was running though some troubles. Patches should be sent today, and I followed Mark's advice on not directly checking errno. Thanks, Mahmoud
Created attachment 232355 [details] Check for readdir errors in fts Fix the bug for fts.c, fts-compat.c, and fts-compat11.c. I followed Mark's advice on setting a separate readdir errno integer in `fts_safe_readdir`, and checking for errors using it. I also preferred to unify the interface of that function regardless of using either `readdir()` or `freebsd11_readdir()`. I would be happy to address any modification requests if there are any.
I'm sorry but I can't find the "Obsoletes" box either in the bug main page or in the "add an attachment" page. Do I need a certain privilege to be able to obsolete someone else's patches?
Hello Mahmoud, Thanks for your the update. Checking the "Obsoletes" box should be done when uploading the patch. I don't think we can do it afterwards (anyway, no worry, we will be able to follow the bug and patch history). Best regards, Ganael.
Quick update : the patches look good to me, but let's wait for a src committer feedback :)
Hello Mark, Thanks for your previous feedback. Could you have a look at Mahmoud's updated patch ? Thanks in advance, Best regards, Ganael.
(In reply to Mahmoud Abumandour from comment #6) Sorry for the delayed reply. This mostly looks ok to me, but I have a few more nits: - Rather than setting errno = 0 in fts_safe_readdir(), I think it's better to set readdir_errno only when readdir(2) fails. - FreeBSD style (see the style(9) man page) is to put declarations at the beginning of the function, so fts_safe_readdir() should declare "ret" at the beginning. - The comment above the readdir_errno check does not seem to be needed.
(In reply to Mark Johnston from comment #11) Thanks for the feedback. I will address it and resend the patch. Regarding setting readdir_errno only if readdir errs, how is this achievable without setting errno to 0? From the man page: The function returns NULL upon reaching the end of the directory or on error. In the event of an error, errno may be set to any of the values documented for the getdirentries(2) system call. Which I think means that we must set errno to 0 to distinguish whether a NULL means error or "end of stream" indicator. Could you please elaborate more? Maybe I can save errno and check that (ret == NULL && saved_errno != errno), and if that's true then set readdir_errno?
Also, is there a patch-checking utility I can use to identify FreeBSD style issues?
(In reply to Mahmoud Abumandour from comment #12) Oops, right, readdir() returns NULL both for errors and for an EOF condition. So you're right, setting errno = 0 is necessary. Sorry for the noise. (In reply to Mahmoud Abumandour from comment #13) Not really, unfortunately, though it's certainly desired. We do have a .clang-format file in the root of the src repository that gets one most of the way there, but it's far from perfect.
Created attachment 232470 [details] Check for readdir errors in fts Addressed feedback by Mark Johnston. I can only obsolete my patches, so there's no way for me to obsolete the initial patch by Ganael.
(In reply to Mahmoud Abumandour from comment #15) Thanks, this looks good to me. I'll do some local testing and queue it up for commit.
(In reply to Mark Johnston from comment #16) Looks like it doesn't compile: fts_safe_readdir() needs to return a struct freebsd11_dirent * in the compat11 code.
Hello, @Mark, thanks for your feedback. @Mahmoud, can you try to fix the patch ? Do you need help with that ? Cheers, Ganael.
Created attachment 232739 [details] Check for readdir errors (take 3) Hello, Here is an updated version of the patch that returns a struct freebsd11_dirent * in the compat11 code. It builds fine with latest head. Best regards, Ganael.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=0cff70ca66547ca5b04030ef07e6a0b9759a0184 commit 0cff70ca66547ca5b04030ef07e6a0b9759a0184 Author: Ganael LAPLANCHE <martymac@FreeBSD.org> AuthorDate: 2022-03-28 14:54:02 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2022-03-28 15:23:46 +0000 libc: Check for readdir(2) errors in fts(3) Previously, such errors were not distinguished from the end-of-directory condition. With improvements from Mahmoud Abumandour <ma.mandourr@gmail.com>. Reviewed by: markj PR: 262038 MFC after: 2 weeks lib/libc/gen/fts-compat.c | 31 ++++++++++++++++++++++++++++--- lib/libc/gen/fts-compat11.c | 32 +++++++++++++++++++++++++++++--- lib/libc/gen/fts.c | 33 ++++++++++++++++++++++++++++++--- 3 files changed, 87 insertions(+), 9 deletions(-)
Thanks Mark!
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=e66bbe6e02b48d1fa57c52321a517dcf42d8eb69 commit e66bbe6e02b48d1fa57c52321a517dcf42d8eb69 Author: Ganael LAPLANCHE <martymac@FreeBSD.org> AuthorDate: 2022-03-28 14:54:02 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2022-04-11 13:43:28 +0000 libc: Check for readdir(2) errors in fts(3) Previously, such errors were not distinguished from the end-of-directory condition. With improvements from Mahmoud Abumandour <ma.mandourr@gmail.com>. Reviewed by: markj PR: 262038 (cherry picked from commit 0cff70ca66547ca5b04030ef07e6a0b9759a0184) lib/libc/gen/fts-compat.c | 31 ++++++++++++++++++++++++++++--- lib/libc/gen/fts-compat11.c | 32 +++++++++++++++++++++++++++++--- lib/libc/gen/fts.c | 33 ++++++++++++++++++++++++++++++--- 3 files changed, 87 insertions(+), 9 deletions(-)