From patchwork Wed Jun 26 20:21:06 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: DJ Delorie X-Patchwork-Id: 33444 Received: (qmail 28330 invoked by alias); 26 Jun 2019 20:21:21 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 28273 invoked by uid 89); 26 Jun 2019 20:21:21 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=objdir, H*i:sk:mvmo92k X-HELO: mx1.redhat.com From: DJ Delorie To: Andreas Schwab Cc: fw@deneb.enyo.de, tuliom@ascii.art.br, libc-alpha@sourceware.org Subject: Re: [PATCHv3] nftw: fill in stat buf for dangling links [BZ #23501] In-Reply-To: (message from Andreas Schwab on Wed, 26 Jun 2019 10:27:14 +0200) Date: Wed, 26 Jun 2019 16:21:06 -0400 Message-ID: MIME-Version: 1.0 Andreas Schwab writes: > Which is what the bug is all about, isn't it? Only FTW_NS is allowed to > keep the stat data undefined. We're returning FTW_NS when we used to return FTW_SLN, in the rare case where readdir sees a dangling symlink which we can't lstat. The bug is about the common case of a dangling symlink that we *can* lstat, where we used to return undefined garbage in the stat_buf but now we return valid data about the link itself (as if FTW_PHYS were set). >> That would mean that programs would see different results due to this >> change, even if they ignored the stat buffer for dangling symlinks. > > There is no dangling symlink, the name does not exist at all. Except the readdir() call returned a symlink. We have a case where readdir() and lstat() are inconsistent. However, it's rare enough that I don't care how it's handled, as long as the lstat-able case works correctly ;-) ---- As per Austin Group interpretation, "the object" wrt a dangling symlink is the symlink itself, despite FTW_PHYS. Reviewed-by: Tulio Magno Quites Machado Filho diff --git a/ChangeLog b/ChangeLog index ac4aba09ba6..2a2da76f2c8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2019-06-26 DJ Delorie + + [BZ #23501] + * io/ftw.c (process_entry): Fill in statbuf for dangling links. + * io/tst-ftw-lnk.c: New test. + * io/Makefile: Run it. + 2019-06-26 Adhemerval Zanella * sysdeps/powerpc/powerpc64/fpu/multiarch/Makefile diff --git a/io/Makefile b/io/Makefile index 088e86da77b..82ed5a00305 100644 --- a/io/Makefile +++ b/io/Makefile @@ -73,7 +73,8 @@ tests := test-utime test-stat test-stat2 test-lfs tst-getcwd \ tst-mknodat tst-mkfifoat tst-ttyname_r bug-ftw5 \ tst-posix_fallocate tst-posix_fallocate64 \ tst-fts tst-fts-lfs tst-open-tmpfile \ - tst-copy_file_range tst-getcwd-abspath tst-lockf + tst-copy_file_range tst-getcwd-abspath tst-lockf \ + tst-ftw-lnk # This test includes the compat implementation of copy_file_range, # which uses internal, unexported libc functions. diff --git a/io/ftw.c b/io/ftw.c index 33e1a5ecab6..fefcf91d1a2 100644 --- a/io/ftw.c +++ b/io/ftw.c @@ -423,10 +423,12 @@ process_entry (struct ftw_data *data, struct dir_data *dir, const char *name, result = -1; else if (data->flags & FTW_PHYS) flag = FTW_NS; - else if (d_type == DT_LNK) - flag = FTW_SLN; else { + /* Old code left ST undefined for dangling DT_LNK without + FTW_PHYS set; a clarification at the POSIX level suggests + it should contain information about the link (ala lstat). + We do our best to fill in what data we can. */ if (dir->streamfd != -1) statres = FXSTATAT (_STAT_VER, dir->streamfd, name, &st, AT_SYMLINK_NOFOLLOW); diff --git a/io/tst-ftw-lnk.c b/io/tst-ftw-lnk.c new file mode 100644 index 00000000000..2a1520b69a8 --- /dev/null +++ b/io/tst-ftw-lnk.c @@ -0,0 +1,239 @@ +/* Test for ftw function related to symbolic links for BZ #23501 + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#define TSTDIR "tst-ftw-lnk.d" + +static void +un (const char *file) +{ + struct stat st; + /* Does the file exist? */ + if (lstat (file, &st) < 0 + && errno == ENOENT) + return; + + /* If so, try to remove it. */ + if (unlink (file) < 0) + FAIL_EXIT1 ("Unable to unlink %s", file); +} + +static void +debug_cb (const char *which, const char *fpath, + const struct stat *sb, int typeflags) +{ + const char *sb_type = "???"; + const char *ftw_type = "???"; + + /* Coding style here is intentionally "wrong" to increase readability. */ + if (S_ISREG (sb->st_mode)) sb_type = "REG"; + if (S_ISDIR (sb->st_mode)) sb_type = "DIR"; + if (S_ISLNK (sb->st_mode)) sb_type = "LNK"; + + if (typeflags == FTW_F) ftw_type = "F"; + if (typeflags == FTW_D) ftw_type = "D"; + if (typeflags == FTW_DNR) ftw_type = "DNR"; + if (typeflags == FTW_DP) ftw_type = "DP"; + if (typeflags == FTW_NS) ftw_type = "NS"; + if (typeflags == FTW_SL) ftw_type = "SL"; + if (typeflags == FTW_SLN) ftw_type = "SLN"; + + printf ("%s %5d %-3s %-3s %s\n", which, (int)(sb->st_ino % 100000), sb_type, ftw_type, fpath); +} + +int good_cb = 0; +#define EXPECTED_GOOD 12 + +/* See if the stat buffer SB refers to the file AS_FNAME. */ +static void +check_same_stats (const struct stat *sb, const char *as_fname) +{ + struct stat as; + if (lstat (as_fname, &as) < 0) + FAIL_EXIT1 ("unable to stat %s for comparison", as_fname); + + if (as.st_mode == sb->st_mode + && as.st_ino == sb->st_ino + && as.st_size == sb->st_size) + good_cb ++; + else + printf ("statbuf data doesn't match %s\n", as_fname); +} + +static int +callback_phys (const char *fpath, const struct stat *sb, int typeflags, struct FTW *ftwbuf) +{ + debug_cb ("P", fpath, sb, typeflags); + + /* This callback is for when the FTW_PHYS flag is set. The results + should reflect the physical filesystem entry, not what it might + point to. */ + + /* link1-bad is a dangling symlink, but we're reporting on the link + anyway (ala lstat ()). */ + if (strcmp (fpath, "./link1-bad") == 0) + { + if (S_ISLNK (sb->st_mode) && typeflags == FTW_SL) + good_cb ++; + else + printf ("link1-bad had wrong phys stats\n"); + + check_same_stats (sb, "link1-bad"); + } + + /* link2-ok is a regular non-dangling symlink. */ + if (strcmp (fpath, "./link2-ok") == 0) + { + if (S_ISLNK (sb->st_mode) && typeflags == FTW_SL) + good_cb ++; + else + printf ("link2-ok had wrong phys stats\n"); + + check_same_stats (sb, "link2-ok"); + } + + /* This is the file link2-ok points to. */ + if (strcmp (fpath, "./link2-tgt") == 0) + { + if (S_ISREG (sb->st_mode) && typeflags == FTW_F) + good_cb ++; + else + printf ("link2-tgt had wrong phys stats\n"); + + check_same_stats (sb, "link2-tgt"); + } + + return 0; +} + +static int +callback_log (const char *fpath, const struct stat *sb, int typeflags, struct FTW *ftwbuf) +{ + debug_cb ("L", fpath, sb, typeflags); + + /* This callback is for when the FTW_PHYS flags is NOT set. The + results should reflect the logical file, i.e. symlinks should be + followed. */ + + /* We would normally report what link1-bad links to, but link1-bad + is a dangling symlink. This is an exception to FTW_PHYS in that + we report FTW_SLN (dangling symlink) but the stat data is + correctly set to the link itself (ala lstat ()). */ + if (strcmp (fpath, "./link1-bad") == 0) + { + if (S_ISLNK (sb->st_mode) && typeflags == FTW_SLN) + good_cb ++; + else + printf ("link1-bad had wrong logical stats\n"); + + check_same_stats (sb, "link1-bad"); + } + + /* link2-ok points to link2-tgt, so we expect data reflecting + link2-tgt (ala stat ()). */ + if (strcmp (fpath, "./link2-ok") == 0) + { + if (S_ISREG (sb->st_mode) && typeflags == FTW_F) + good_cb ++; + else + printf ("link2-ok had wrong logical stats\n"); + + check_same_stats (sb, "link2-tgt"); + } + + /* This is the file link2-ok points to. */ + if (strcmp (fpath, "./link2-tgt") == 0) + { + if (S_ISREG (sb->st_mode) && typeflags == FTW_F) + good_cb ++; + else + printf ("link2-tgt had wrong logical stats\n"); + + check_same_stats (sb, "link2-tgt"); + } + + return 0; +} + +static int +do_test (void) +{ + struct stat st; + + if (chdir (support_objdir_root) < 0) + FAIL_EXIT1 ("cannot chdir to objdir root"); + + if (chdir ("io") < 0) + FAIL_EXIT1 ("cannot chdir to objdir/io subdir"); + + if (stat (TSTDIR, &st) >= 0) + { + /* Directory does exist, delete any potential conflicts. */ + if (chdir (TSTDIR) < 0) + FAIL_EXIT1 ("cannot chdir to %s\n", TSTDIR); + un ("link1-bad"); + un ("link1-tgt"); + un ("link2-ok"); + un ("link2-tgt"); + } + else + { + /* Directory does not exist, create it. */ + mkdir (TSTDIR, 0777); + if (chdir (TSTDIR) < 0) + FAIL_EXIT1 ("cannot chdir to %s\n", TSTDIR); + } + + /* At this point, we're inside our test directory, and need to + prepare it. */ + + if (symlink ("link1-tgt", "link1-bad") < 0) + FAIL_EXIT1 ("symlink link1-bad failed"); + if (symlink ("link2-tgt", "link2-ok") < 0) + FAIL_EXIT1 ("symlink link2-ok failed"); + if (open ("link2-tgt", O_RDWR|O_CREAT, 0777) < 0) + FAIL_EXIT1 ("create of link2-tgt failed"); + + /* Now we run the tests. */ + + nftw (".", callback_phys, 10, FTW_PHYS); + nftw (".", callback_log, 10, 0); + + /* Did we see the expected number of correct callbacks? */ + + if (good_cb != EXPECTED_GOOD) + { + FAIL_EXIT1 ("Saw %d good callbacks, expected %d\n", + good_cb, EXPECTED_GOOD); + } + + return 0; +} + +#include