| Message ID | 1504643122-14874-10-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | posix: glob fixes and refactor | expand |
Although this is a definite bug and the patch fixes this instance of it, I'm
afraid other instances remain unfixed. For example:
glob_t g; glob ("//a*b", 0, NULL, &g)
can do the wrong thing, since glob calls opendir on "/" instead of "//", and on
some platforms "/" and "//" are different directories (POSIX allows this as a
special exception).
A more serious example. If you do this:
ln -s /no-such-file globlink1
ln -s . globlink2
then:
glob_t g;
int res = glob ("globlink[12]/", 0, NULL, &g);
assert (res == 0 && g.gl_pathc == 1);
assert (strcmp (g.gl_pathv[0], "globlink2/") == 0);
fails, since glob gets confused about directories and slashes and mistakenly
returns two results. Although this bug is seemingly unrelated, the underlying
cause is the same: glob gets confused about whether to include or exclude
slashes when doing its tests.
I'll take a look at it, though the fix won't be trivial.
PS. This finishes my review of this patchset. Patches 1-8 are OK to be
installed, with the trivial changes I suggested earlier. This patch (patch 9)
I'd like to hold off on, until we've had a chance to work out a
more-comprehensive fix.
PPS. I'm still slowly wending my way through your original patchset. Most
recently I looked at "[PATCH 07/18] posix: User LOGIN_NAME_MAX for all user
names in glob" <https://sourceware.org/ml/libc-alpha/2017-08/msg00447.html>. I'm
afraid a good fix needs to be hairier there too, as POSIX does not require
LOGIN_NAME_MAX to be suitable for a stack-based buffer, or even to be defined. I
have a partly-drafted patch which I hope to finish in the not-too-distant future.
On 08/09/2017 00:14, Paul Eggert wrote: > Although this is a definite bug and the patch fixes this instance of > it, I'm afraid other instances remain unfixed. For example: > > glob_t g; glob ("//a*b", 0, NULL, &g) > > can do the wrong thing, since glob calls opendir on "/" instead of > "//", and on some platforms "/" and "//" are different directories > (POSIX allows this as a special exception). > > A more serious example. If you do this: > > ln -s /no-such-file globlink1 > ln -s . globlink2 > > then: > > glob_t g; > int res = glob ("globlink[12]/", 0, NULL, &g); > assert (res == 0 && g.gl_pathc == 1); > assert (strcmp (g.gl_pathv[0], "globlink2/") == 0); > > fails, since glob gets confused about directories and slashes and > mistakenly returns two results. Although this bug is seemingly > unrelated, the underlying cause is the same: glob gets confused about > whether to include or exclude slashes when doing its tests. > > I'll take a look at it, though the fix won't be trivial. > > PS. This finishes my review of this patchset. Patches 1-8 are OK to be > installed, with the trivial changes I suggested earlier. This patch > (patch 9) I'd like to hold off on, until we've had a chance to work > out a more-comprehensive fix. Fair enough, I will hold patch 9 push and take a look at the examples you brought up. Thanks for the follow up. > > PPS. I'm still slowly wending my way through your original patchset. > Most recently I looked at "[PATCH 07/18] posix: User LOGIN_NAME_MAX > for all user names in glob" > <https://sourceware.org/ml/libc-alpha/2017-08/msg00447.html>. I'm > afraid a good fix needs to be hairier there too, as POSIX does not > require LOGIN_NAME_MAX to be suitable for a stack-based buffer, or > even to be defined. I have a partly-drafted patch which I hope to > finish in the not-too-distant future. Alright, my initial patch was to adequate it to glibc code (which does define a actual limit suitable to stack allocation) and get rid of any alloca usage. My understanding, based on gnulib commit 064df0b0c, is it should not impose a limit on user name length. So currently in a patchset I am intended to send after this one, user name handling is now based on my char_array struct and thus allocates the user_name dinamically if required.
diff --git a/posix/glob.c b/posix/glob.c index 30a4143..25c5d24 100644 --- a/posix/glob.c +++ b/posix/glob.c @@ -272,6 +272,8 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int), size_t oldcount; int meta; int dirname_modified; + /* Indicate if the directory should be prepended on return values. */ + bool dirname_prefix = true; int malloc_dirname = 0; glob_t dirs; int retval = 0; @@ -495,6 +497,10 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int), dirname = (char *) "/"; dirlen = 1; ++filename; + /* prefix_array adds a separator for each result and DIRNAME is + already '/'. So we indicate later that we should not prepend + anything for this specific case. */ + dirname_prefix = false; } else { @@ -1086,7 +1092,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int), if (dirlen > 0) { /* Stick the directory on the front of each name. */ - if (prefix_array (dirname, + if (prefix_array (dirname_prefix ? dirname : "", &pglob->gl_pathv[old_pathc + pglob->gl_offs], pglob->gl_pathc - old_pathc)) { @@ -1167,11 +1173,6 @@ prefix_array (const char *dirname, char **array, size_t n) size_t dirlen = strlen (dirname); char dirsep_char = '/'; - if (dirlen == 1 && dirname[0] == '/') - /* DIRNAME is just "/", so normal prepending would get us "//foo". - We want "/foo" instead, so don't prepend any chars from DIRNAME. */ - dirlen = 0; - #if defined __MSDOS__ || defined WINDOWS32 if (dirlen > 1) { diff --git a/posix/globtest.sh b/posix/globtest.sh index 73f7ae3..92a8e37 100755 --- a/posix/globtest.sh +++ b/posix/globtest.sh @@ -242,6 +242,43 @@ if test $failed -ne 0; then result=1 fi +# Test NOCHECK for specific cases where the pattern used starts +# with '/' (BZ#10246). +failed=0 +${test_program_prefix} \ +${common_objpfx}posix/globtest -c "$testdir" "/%" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`/%' +EOF +if test $failed -ne 0; then + echo "No check test failed" >> $logfile + result=1 +fi + +${test_program_prefix} \ +${common_objpfx}posix/globtest -c "$testdir" "//%" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`//%' +EOF +if test $failed -ne 0; then + echo "No check test failed" >> $logfile + result=1 +fi + +${test_program_prefix} \ +${common_objpfx}posix/globtest -c "$testdir" "///%" | +sort > $testout +cat <<"EOF" | $CMP - $testout >> $logfile || failed=1 +`///%' +EOF +if test $failed -ne 0; then + echo "No check test failed" >> $logfile + result=1 +fi + + # Test NOMAGIC without magic characters failed=0 ${test_program_prefix} \