[3/9] posix: Allow glob to match dangling symlinks [BZ #866]
Commit Message
This patch makes glob match dangling symlinks. Compared to other glob
implementation (*BSD, bash, musl, and other shells as well), GLIBC seems
the be the only one that does not match dangling symlinks. As for
comment #5 in BZ #866, POSIX does not have any strict specification for
dangling symlinks match and it is reasonable that trying to glob everything
in a path should return all types of files (such as for a 'rm *'). Also,
comment #7 shows even more example where GLIBC current behavior is
unexepected.
I avoided adding another GNU specific flag to set this behavior and
instead make it the default. Although this change the semanthic from
previous implementation, I think adding another compat symbol to be
really unecessary as from aforementioned reasons (current behavior not
defined in any standard, general idea of different implementation is
to list dangling symbols).
This also sync glob with gnulib commit fd1daf4 (glob: match dangling
symlinks).
Checked on x86_64-linux-gnu and on a build using build-many-glibcs.py
for all major architectures.
[BZ #866]
[BZ #1062]
* posix/Makefile (tests): Remove bug-glob1 and tst-glob_symlinks.
* posix/bug-glob1.c: Remove file.
* posix/tst-glob_symlinks.c: New file.
* lib/glob.c (__lstat64): New macro.
(is_dir): New function.
(glob, glob_in_dir): Match symlinks even if they are dangling.
(link_stat, link_exists_p): Remove. All uses removed.
---
ChangeLog | 10 ++
posix/Makefile | 4 +-
posix/bug-glob1.c | 88 -----------------
posix/glob.c | 239 +++++++++++++++++-----------------------------
posix/tst-glob_symlinks.c | 132 +++++++++++++++++++++++++
5 files changed, 230 insertions(+), 243 deletions(-)
delete mode 100644 posix/bug-glob1.c
create mode 100644 posix/tst-glob_symlinks.c
Comments
'git am' complains about this patch because it introduces a line with a space
before a tab.
Thanks I fixed it in my repo.
On 05/09/2017 22:27, Paul Eggert wrote:
> 'git am' complains about this patch because it introduces a line with a space before a tab.
This breaks make, it doesn't expect that glob calls gl_lstat.
dir_setup_glob:
/* We don't bother setting gl_lstat, since glob never calls it.
The slot is only there for compatibility with 4.4 BSD. */
Andreas.
I give you that we should properly document what GLOB_ALTDIRFUNC
expects for partially initialized glob_t alternative functions, but currenyl
glob code assumes that if GLOB_ALTDIRFUNC is set then glob_t
function pointers actually points to valid implementation.
So I think this is essentially a make issue. And since make also packs
its own glob copy from gnulib, it is matter to fix on make if and when
it syncs with gnulib.
On 09/09/2017 11:50, Andreas Schwab wrote:
> This breaks make, it doesn't expect that glob calls gl_lstat.
>
> dir_setup_glob:
>
> /* We don't bother setting gl_lstat, since glob never calls it.
> The slot is only there for compatibility with 4.4 BSD. */
>
> Andreas.
>
Adhemerval Zanella wrote:
> since make also packs
> its own glob copy from gnulib, it is matter to fix on make if and when
> it syncs with gnulib.
No, GNU Make uses glibc glob if it passes the compatibility tests in
'configure', which it does. So previously-built instances of GNU make will
likely crash if run with a glibc containing the proposed symlink changes. Even
if you rebuild GNU Make from scratch it will still crash, because glibc glob
will pass GNU Make's tests even with the patch.
We could fix this by incrementing _GNU_GLOB_INTERFACE_VERSION to 2 (causing GNU
Make's configure-time test to fail), but this is a serious step that requires
changing the libc.so major version number, creating backwards-compatibility
functions for the old behavior, etc. I doubt whether the symlink glitch with
'glob' is worth all this effort.
How about the following idea instead: establish two new flags GLOB_FOLLOW and
GLOB_NOFOLLOW, where the caller specifies whether symlinks should be followed.
The default is system-dependent. For glibc the default is GLOB_FOLLOW (we can
even make GLOB_FOLLOW zero). For FreeBSD the default would be GLOB_NOFOLLOW,
assuming they like the idea of supporting these flags. This maintains
backward-compatibility for both kinds of platforms. For application code
preferring GLOB_NOFOLLOW semantics if available, a simple:
#include <glob.h>
#ifndef GLOB_NOFOLLOW
# define GLOB_NOFOLLOW 0
#endif
will do, as long as all calls go glob specify 'GLOB_NOFOLLOW'. We can implement
this idea first in Gnulib and then propose it for glibc.
Anyway, I'll submit a bug report to GNU Make, since it should not be assuming
this implementation detail of glibc, regardless of what we decide about the
above matter. However, it will be at best many years before we can assume this
bug is fixed in the wild.
On Sat, Sep 9, 2017 at 1:01 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> How about the following idea instead: establish two new flags GLOB_FOLLOW
> and GLOB_NOFOLLOW, where the caller specifies whether symlinks should be
> followed. The default is system-dependent. For glibc the default is
> GLOB_FOLLOW (we can even make GLOB_FOLLOW zero). For FreeBSD the default
> would be GLOB_NOFOLLOW, assuming they like the idea of supporting these
> flags. This maintains backward-compatibility for both kinds of platforms.
> For application code preferring GLOB_NOFOLLOW semantics if available, a
> simple:
>
> #include <glob.h>
> #ifndef GLOB_NOFOLLOW
> # define GLOB_NOFOLLOW 0
> #endif
>
> will do, as long as all calls go glob specify 'GLOB_NOFOLLOW'. We can
> implement this idea first in Gnulib and then propose it for glibc.
This also sounds like a lot of complexity. With the bug in Make, is
gl_lstat garbage or is it NULL? We could have the glob implementation
check for NULL.
zw
On Sep 09 2017, Zack Weinberg <zackw@panix.com> wrote:
> This also sounds like a lot of complexity. With the bug in Make, is
> gl_lstat garbage or is it NULL?
It is uninitialized.
Andreas.
On Sat, Sep 9, 2017 at 1:26 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Sep 09 2017, Zack Weinberg <zackw@panix.com> wrote:
>
>> This also sounds like a lot of complexity. With the bug in Make, is
>> gl_lstat garbage or is it NULL?
>
> It is uninitialized.
Drat.
On 09/09/2017 19:01, Paul Eggert wrote:
> Adhemerval Zanella wrote:
>> since make also packs
>> its own glob copy from gnulib, it is matter to fix on make if and when
>> it syncs with gnulib.
>
> No, GNU Make uses glibc glob if it passes the compatibility tests in
> 'configure', which it does. So previously-built instances of GNU make
> will likely crash if run with a glibc containing the proposed symlink
> changes. Even if you rebuild GNU Make from scratch it will still
> crash, because glibc glob will pass GNU Make's tests even with the patch.
>
> We could fix this by incrementing _GNU_GLOB_INTERFACE_VERSION to 2
> (causing GNU Make's configure-time test to fail), but this is a
> serious step that requires changing the libc.so major version number,
> creating backwards-compatibility functions for the old behavior, etc.
> I doubt whether the symlink glitch with 'glob' is worth all this effort.
>
> How about the following idea instead: establish two new flags
> GLOB_FOLLOW and GLOB_NOFOLLOW, where the caller specifies whether
> symlinks should be followed. The default is system-dependent. For
> glibc the default is GLOB_FOLLOW (we can even make GLOB_FOLLOW zero).
> For FreeBSD the default would be GLOB_NOFOLLOW, assuming they like the
> idea of supporting these flags. This maintains backward-compatibility
> for both kinds of platforms. For application code preferring
> GLOB_NOFOLLOW semantics if available, a simple:
>
> #include <glob.h>
> #ifndef GLOB_NOFOLLOW
> # define GLOB_NOFOLLOW 0
> #endif
>
> will do, as long as all calls go glob specify 'GLOB_NOFOLLOW'. We can
> implement this idea first in Gnulib and then propose it for glibc.
>
> Anyway, I'll submit a bug report to GNU Make, since it should not be
> assuming this implementation detail of glibc, regardless of what we
> decide about the above matter. However, it will be at best many years
> before we can assume this bug is fixed in the wild.
I would prefer to avoid adding a new flag, but for this specific issue I
do not see a better
solution (as you have said I also agree bumping interface version does
not worth the
trouble). What really bothers me is the motivation to actually support
it is to maintain
compatibility with a undefined use of a not well documented interface
(which imho is
clearly a bug in 'make' usage). This will be another adhoc gnu
extension, which
most likely won't be used anywhere besides on make itself (and the
system-dependent
semantic will also lead to more confusion).
Another option is to add a compat glob symbol with previous semantic
(without
actually bumping _GNU_GLOB_INTERFACE_VERSION). It still won't help new
'make'
builds against newer glibc (not without fixing make anyway), so I am not
sure if
is feasible solution.
Adhemerval Zanella wrote:
> I would prefer to avoid adding a new flag,
Me too.
> Another option is to add a compat glob symbol with previous semantic
> (without
> actually bumping _GNU_GLOB_INTERFACE_VERSION).
I like this option better than what I suggested. How about if you propose a
patch along those lines?
PS. I sent in a bug report for GNU Make about a half hour ago. See:
http://lists.gnu.org/archive/html/bug-make/2017-09/msg00014.html
On Sun, 10 Sep 2017, Adhemerval Zanella wrote:
> Another option is to add a compat glob symbol with previous semantic
> (without actually bumping _GNU_GLOB_INTERFACE_VERSION). It still won't
To be clear, that's adding new symbol versions of glob and glob64
everywhere, making all existing versions (some configurations already have
more than one version of glob or glob64) ignore gl_lstat for
compatibility.
On Mon, Sep 11, 2017 at 10:33 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Sun, 10 Sep 2017, Adhemerval Zanella wrote:
>
>> Another option is to add a compat glob symbol with previous semantic
>> (without actually bumping _GNU_GLOB_INTERFACE_VERSION). It still won't
>
> To be clear, that's adding new symbol versions of glob and glob64
> everywhere, making all existing versions (some configurations already have
> more than one version of glob or glob64) ignore gl_lstat for
> compatibility.
If this is done, the new symbol version should attempt to validate the
altdirfuncs structure on entry, so that the buggy versions of make
(and any other programs with the same bug) get a reliable failure
rather than crashing only if glob thinks it needs to call gl_lstat.
Otherwise it'll be like the memcpy mess, where people didn't notice
that they had a bug they needed to fix.
zw
On 09/11/2017 07:38 AM, Zack Weinberg wrote:
> If this is done, the new symbol version should attempt to validate the
> altdirfuncs structure on entry
I don't see how glob could do that reliably and cheaply. How does one
validate that a pointer to a stat-like function is actually a pointer to
a stat-like function? All one can do is call the function and pray.
On Mon, Sep 11, 2017 at 12:53 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 09/11/2017 07:38 AM, Zack Weinberg wrote:
>>
>> If this is done, the new symbol version should attempt to validate the
>> altdirfuncs structure on entry
>
>
> I don't see how glob could do that reliably and cheaply. How does one
> validate that a pointer to a stat-like function is actually a pointer to a
> stat-like function? All one can do is call the function and pray.
It should be enough to make a dummy call, e.g.
pglob->gl_lstat(".", &statbuf);
zw
On 09/11/2017 10:25 AM, Zack Weinberg wrote:
>
> It should be enough to make a dummy call, e.g.
>
> pglob->gl_lstat(".", &statbuf);
>
Unfortunately calling lstat is quite expensive on some (non-POSIX)
platforms, even on the working directory. So we can't do the above in
the Gnulib version. Besides, under the proposed patch glob is going to
use gl_lstat instead of gl_stat in almost all cases, so the dummy call
won't add much extra checking.
I suppose we could valid gl_stat instead, as gl_stat usage will become
rare (used only if GLOB_MARK is also specified, just before returning
results). But we don't have any code in the wild that is giving us
invalid gl_stat pointers, so it wouldn't be that helpful to try to
validate gl_stat either.
On Mon, Sep 11, 2017 at 1:38 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 09/11/2017 10:25 AM, Zack Weinberg wrote:
>>
>>
>> It should be enough to make a dummy call, e.g.
>>
>> pglob->gl_lstat(".", &statbuf);
>>
>
> Unfortunately calling lstat is quite expensive on some (non-POSIX)
> platforms, even on the working directory. So we can't do the above in the
> Gnulib version.
I have trouble believing this will be a measurable performance hit,
considering how much other expensive work glob has to do.
> Besides, under the proposed patch glob is going to use
> gl_lstat instead of gl_stat in almost all cases, so the dummy call won't add
> much extra checking.
The point is not to add _extra_ checking; the point is to ensure that
gl_lstat (and gl_stat) are valid on all calls, _even if_ they wouldn't
otherwise have been used. I'm trying to turn "may fail at runtime
under rare circumstances" into "will definitely fail at runtime on the
first use", which is the best we can do in C.
> I suppose we could valid gl_stat instead, as gl_stat usage will become rare
> (used only if GLOB_MARK is also specified, just before returning results).
> But we don't have any code in the wild that is giving us invalid gl_stat
> pointers, so it wouldn't be that helpful to try to validate gl_stat either.
So here's an alternative, less thorough but perhaps also less costly
approach: when GLOB_ALTDIRFUNCS is set, call both gl_stat and gl_lstat
on the first name that's going to be returned, even if we have no
other reason to do this. Optionally, memoize the function pointer and
don't bother making the extra call again if we recognize that it's
known to work.
(Maybe also it would be a good idea to check up front for any NULL
callbacks in the ALTDIRFUNCS case.)
zw
On 09/11/2017 10:56 AM, Zack Weinberg wrote:
> So here's an alternative, less thorough but perhaps also less costly
> approach: when GLOB_ALTDIRFUNCS is set, call both gl_stat and gl_lstat
> on the first name that's going to be returned, even if we have no
> other reason to do this.
Something like that would be better, yes. Still not sure it's worth the
trouble. We don't know how expensive gl_stat and gl_lstat will be, in
general.
On 11/09/2017 15:03, Paul Eggert wrote:
> On 09/11/2017 10:56 AM, Zack Weinberg wrote:
>> So here's an alternative, less thorough but perhaps also less costly
>> approach: when GLOB_ALTDIRFUNCS is set, call both gl_stat and gl_lstat
>> on the first name that's going to be returned, even if we have no
>> other reason to do this.
> Something like that would be better, yes. Still not sure it's worth the trouble. We don't know how expensive gl_stat and gl_lstat will be, in general.
Another approach that does not involve adding compat symbols (which adds
a lot of code complexity inside glibc build and do not solve 'make' builds
against new glibc) would to make GLOB_ALTDIRFUNCS to follow the old semantic
of using gl_stat instead of gl_lstat while making glob without GLOB_ALTDIRFUNCS
works as intended. And add another flag, GLOB_ALTDIRFUNCS2, which actually
uses gl_lstat.
It will solve make compat issue even for build against newer glibcs with
the cost of making GLOB_ALTDIRFUNCS with a slight different semantic than
default glob (which given the current situation is a feasible cost).
Adhemerval Zanella wrote:
> Another approach that does not involve adding compat symbols (which adds
> a lot of code complexity inside glibc build and do not solve 'make' builds
> against new glibc) would to make GLOB_ALTDIRFUNCS to follow the old semantic
> of using gl_stat instead of gl_lstat while making glob without GLOB_ALTDIRFUNCS
> works as intended. And add another flag, GLOB_ALTDIRFUNCS2, which actually
> uses gl_lstat.
Although that's clever, it is a gratuitous source-code incompatibility with BSD,
which is not a good thing. To some extent it's just GLOB_FOLLOW and
GLOB_NOFOLLOW in disguise, and disguise is not a good thing in APIs. So I think
I still prefer the compat symbol approach.
We'll get GNU 'Make' fixed, and I wouldn't worry overly much about people
building unpatched 'Make' with new glibc. I filed a Make bug report is here:
http://lists.gnu.org/archive/html/bug-make/2017-09/msg00014.html
On 13/09/2017 06:14, Paul Eggert wrote:
> Adhemerval Zanella wrote:
>> Another approach that does not involve adding compat symbols (which adds
>> a lot of code complexity inside glibc build and do not solve 'make' builds
>> against new glibc) would to make GLOB_ALTDIRFUNCS to follow the old semantic
>> of using gl_stat instead of gl_lstat while making glob without GLOB_ALTDIRFUNCS
>> works as intended. And add another flag, GLOB_ALTDIRFUNCS2, which actually
>> uses gl_lstat.
>
> Although that's clever, it is a gratuitous source-code incompatibility with BSD, which is not a good thing. To some extent it's just GLOB_FOLLOW and GLOB_NOFOLLOW in disguise, and disguise is not a good thing in APIs. So I think I still prefer the compat symbol approach.
>
> We'll get GNU 'Make' fixed, and I wouldn't worry overly much about people building unpatched 'Make' with new glibc. I filed a Make bug report is here:
>
> http://lists.gnu.org/archive/html/bug-make/2017-09/msg00014.html
Right, I am mainly trying to avoid bring more internal glob implementation
complexity to glibc, but since you says the unpatched 'Make' built
against newer glibc shouldn't be a problem I think we can this way.
I will work on it.
On 13/09/17 13:22, Adhemerval Zanella wrote:
> On 13/09/2017 06:14, Paul Eggert wrote:
>> Adhemerval Zanella wrote:
>>> Another approach that does not involve adding compat symbols (which adds
>>> a lot of code complexity inside glibc build and do not solve 'make' builds
>>> against new glibc) would to make GLOB_ALTDIRFUNCS to follow the old semantic
>>> of using gl_stat instead of gl_lstat while making glob without GLOB_ALTDIRFUNCS
>>> works as intended. And add another flag, GLOB_ALTDIRFUNCS2, which actually
>>> uses gl_lstat.
>>
>> Although that's clever, it is a gratuitous source-code incompatibility with BSD, which is not a good thing. To some extent it's just GLOB_FOLLOW and GLOB_NOFOLLOW in disguise, and disguise is not a good thing in APIs. So I think I still prefer the compat symbol approach.
>>
>> We'll get GNU 'Make' fixed, and I wouldn't worry overly much about people building unpatched 'Make' with new glibc. I filed a Make bug report is here:
>>
>> http://lists.gnu.org/archive/html/bug-make/2017-09/msg00014.html
>
> Right, I am mainly trying to avoid bring more internal glob implementation
> complexity to glibc, but since you says the unpatched 'Make' built
> against newer glibc shouldn't be a problem I think we can this way.
> I will work on it.
>
i think breaking make is a serious issue now for
anyone trying to do toolchain dev (in native chroots).
and if this gets into a released version of glibc
then it will be an issue for distros.
i think old make binaries should keep working with
glibc 2.27 whatever it takes and it's best to fix
this breakage sooner than later (it's a pain to
carry patched make around).
(this is not different than the stupid malloc hook
usage was in emacs and that was dragged out over
several years until glibc was fixed to wait for
a stable emacs release that is fixed, the same
should be done for make)
On 14/09/2017 07:05, Szabolcs Nagy wrote:
> On 13/09/17 13:22, Adhemerval Zanella wrote:
>> On 13/09/2017 06:14, Paul Eggert wrote:
>>> Adhemerval Zanella wrote:
>>>> Another approach that does not involve adding compat symbols (which adds
>>>> a lot of code complexity inside glibc build and do not solve 'make' builds
>>>> against new glibc) would to make GLOB_ALTDIRFUNCS to follow the old semantic
>>>> of using gl_stat instead of gl_lstat while making glob without GLOB_ALTDIRFUNCS
>>>> works as intended. And add another flag, GLOB_ALTDIRFUNCS2, which actually
>>>> uses gl_lstat.
>>>
>>> Although that's clever, it is a gratuitous source-code incompatibility with BSD, which is not a good thing. To some extent it's just GLOB_FOLLOW and GLOB_NOFOLLOW in disguise, and disguise is not a good thing in APIs. So I think I still prefer the compat symbol approach.
>>>
>>> We'll get GNU 'Make' fixed, and I wouldn't worry overly much about people building unpatched 'Make' with new glibc. I filed a Make bug report is here:
>>>
>>> http://lists.gnu.org/archive/html/bug-make/2017-09/msg00014.html
>>
>> Right, I am mainly trying to avoid bring more internal glob implementation
>> complexity to glibc, but since you says the unpatched 'Make' built
>> against newer glibc shouldn't be a problem I think we can this way.
>> I will work on it.
>>
>
> i think breaking make is a serious issue now for
> anyone trying to do toolchain dev (in native chroots).
> and if this gets into a released version of glibc
> then it will be an issue for distros.
>
> i think old make binaries should keep working with
> glibc 2.27 whatever it takes and it's best to fix
> this breakage sooner than later (it's a pain to
> carry patched make around).
Yes, that was the consensus and the idea is to provide a compat symbol
that does not call gl_lstat.
As a side note, make tests itself does not trigger it this issue
(running make tests with a newer glibc shows no regression), so it
would be good also if make adds a newer tests to stress it.
>
> (this is not different than the stupid malloc hook
> usage was in emacs and that was dragged out over
> several years until glibc was fixed to wait for
> a stable emacs release that is fixed, the same
> should be done for make)
>
On 09/13/2017 11:14 AM, Paul Eggert wrote:
> Although that's clever, it is a gratuitous source-code incompatibility
> with BSD, which is not a good thing. To some extent it's just
> GLOB_FOLLOW and GLOB_NOFOLLOW in disguise, and disguise is not a good
> thing in APIs. So I think I still prefer the compat symbol approach.
If the BSDs are currently source-code-compatible, why doesn't GNU make
fail there already?
Thanks,
Florian
On 15/09/2017 17:18, Florian Weimer wrote:
> On 09/13/2017 11:14 AM, Paul Eggert wrote:
>> Although that's clever, it is a gratuitous source-code incompatibility with BSD, which is not a good thing. To some extent it's just GLOB_FOLLOW and GLOB_NOFOLLOW in disguise, and disguise is not a good thing in APIs. So I think I still prefer the compat symbol approach.
>
> If the BSDs are currently source-code-compatible, why doesn't GNU make fail there already?
My understanding is BSDs were not current source-code-compatible before
the dangling symlink fix (commit 5554304f0) since afaik both openbsd
and freebsd do check for dangling symlinks (using gl_lstat if it is
the case).
Reverting back to ol GLOB_ALTDIRFUNC semantic with make glibc again
source-code-incompatible and by adding an extra flag would require
adjustments in the program source code to actually handle it.
Florian Weimer wrote:
> If the BSDs are currently source-code-compatible, why doesn't GNU make fail
> there already?
Because GNU make never uses BSD glob. GNU make's 'configure' script checks that
_GNU_GLOB_INTERFACE_VERSION equals 1, and if not it compiles and uses its own
glob implementation (copied from an old version of glibc).
If glibc changed _GNU_GLOB_INTERFACE_VERSION to 2, old versions of GNU make
would start rejecting new versions of glibc, and so would build and run OK
because they'd use their old copy of glob. The comment in gnu-versions.h says
that if we change _GNU_GLOB_INTERFACE_VERSION then we must change the libc.so
major version, but this rule seems arbitrary.
Suppose we ignore the gnu-versions.h comment and update
_GNU_GLOB_INTERFACE_VERSION to 2 without changing libc.so's major version.
Wouldn't this fix the compatibility problem with GNU Make?
On 09/17/2017 09:16 AM, Paul Eggert wrote:
> Florian Weimer wrote:
>> If the BSDs are currently source-code-compatible, why doesn't GNU make
>> fail there already?
>
> Because GNU make never uses BSD glob. GNU make's 'configure' script
> checks that _GNU_GLOB_INTERFACE_VERSION equals 1, and if not it compiles
> and uses its own glob implementation (copied from an old version of glibc).
Ah, thanks.
> If glibc changed _GNU_GLOB_INTERFACE_VERSION to 2, old versions of GNU
> make would start rejecting new versions of glibc, and so would build and
> run OK because they'd use their old copy of glob. The comment in
> gnu-versions.h says that if we change _GNU_GLOB_INTERFACE_VERSION then
> we must change the libc.so major version, but this rule seems arbitrary.
This comment predates the availability of symbol versioning. It was
true when it was written.
> Suppose we ignore the gnu-versions.h comment and update
> _GNU_GLOB_INTERFACE_VERSION to 2 without changing libc.so's major
> version. Wouldn't this fix the compatibility problem with GNU Make?
In addition to adding a compat symbols? Yes, that could work.
I don't really like this situation, but this combination seems to be a
somewhat reasonable way to fix both the glob bug and preserve backwards
compatibility.
Thanks,
Florian
> Il giorno 17 set 2017, alle ore 04:48, Florian Weimer <fweimer@redhat.com> ha scritto:
>
>> On 09/17/2017 09:16 AM, Paul Eggert wrote:
>> Florian Weimer wrote:
>>> If the BSDs are currently source-code-compatible, why doesn't GNU make fail there already?
>> Because GNU make never uses BSD glob. GNU make's 'configure' script checks that _GNU_GLOB_INTERFACE_VERSION equals 1, and if not it compiles and uses its own glob implementation (copied from an old version of glibc).
>
> Ah, thanks.
>
>> If glibc changed _GNU_GLOB_INTERFACE_VERSION to 2, old versions of GNU make would start rejecting new versions of glibc, and so would build and run OK because they'd use their old copy of glob. The comment in gnu-versions.h says that if we change _GNU_GLOB_INTERFACE_VERSION then we must change the libc.so major version, but this rule seems arbitrary.
>
> This comment predates the availability of symbol versioning. It was true when it was written.
>
>> Suppose we ignore the gnu-versions.h comment and update _GNU_GLOB_INTERFACE_VERSION to 2 without changing libc.so's major version. Wouldn't this fix the compatibility problem with GNU Make?
>
> In addition to adding a compat symbols? Yes, that could work.
>
> I don't really like this situation, but this combination seems to be a somewhat reasonable way to fix both the glob bug and preserve backwards compatibility.
Alright, I am also not very found on the required hacking to fix make, but I agree this seems to be the safest path. I will update the glob compat patch with minor fixes pointed out by Joseph and the _GNU_GLOB_INTERFACE_VERSION version bump.
@@ -79,7 +79,7 @@ tests := test-errno tstgetopt testfnm runtests runptests \
tst-nice tst-nanosleep tst-regex2 \
transbug tst-rxspencer tst-pcre tst-boost \
bug-ga1 tst-vfork1 tst-vfork2 tst-vfork3 tst-waitid \
- tst-getaddrinfo2 bug-glob1 bug-glob2 bug-glob3 tst-sysconf \
+ tst-getaddrinfo2 bug-glob2 bug-glob3 tst-sysconf \
tst-execvp1 tst-execvp2 tst-execlp1 tst-execlp2 \
tst-execv1 tst-execv2 tst-execl1 tst-execl2 \
tst-execve1 tst-execve2 tst-execle1 tst-execle2 \
@@ -93,7 +93,7 @@ tests := test-errno tstgetopt testfnm runtests runptests \
tst-fnmatch3 bug-regex36 tst-getaddrinfo5 \
tst-posix_spawn-fd tst-posix_spawn-setsid \
tst-posix_fadvise tst-posix_fadvise64 \
- tst-sysconf-empty-chroot
+ tst-sysconf-empty-chroot tst-glob_symlinks
tests-internal := bug-regex5 bug-regex20 bug-regex33 \
tst-rfc3484 tst-rfc3484-2 tst-rfc3484-3
xtests := bug-ga2
deleted file mode 100644
@@ -1,88 +0,0 @@
-/* Test case for globbing dangling symlink. By Ulrich Drepper. */
-#include <errno.h>
-#include <error.h>
-#include <glob.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-
-
-static void prepare (int argc, char *argv[]);
-#define PREPARE prepare
-static int do_test (void);
-#define TEST_FUNCTION do_test ()
-
-#include "../test-skeleton.c"
-
-
-static char *fname;
-
-static void
-prepare (int argc, char *argv[])
-{
- if (argc < 2)
- error (EXIT_FAILURE, 0, "missing argument");
-
- size_t len = strlen (argv[1]);
- static const char ext[] = "globXXXXXX";
- fname = malloc (len + sizeof (ext));
- if (fname == NULL)
- error (EXIT_FAILURE, errno, "cannot create temp file");
- again:
- strcpy (stpcpy (fname, argv[1]), ext);
- fname = mktemp (fname);
- if (fname == NULL || *fname == '\0')
- error (EXIT_FAILURE, errno, "cannot create temp file name");
- if (symlink ("bug-glob1-does-not-exist", fname) != 0)
- {
- if (errno == EEXIST)
- goto again;
-
- error (EXIT_FAILURE, errno, "cannot create symlink");
- }
- add_temp_file (fname);
-}
-
-
-static int
-do_test (void)
-{
- glob_t gl;
- int retval = 0;
- int e;
-
- e = glob (fname, 0, NULL, &gl);
- if (e == 0)
- {
- printf ("glob(\"%s\") succeeded\n", fname);
- retval = 1;
- }
- globfree (&gl);
-
- size_t fnamelen = strlen (fname);
- char buf[fnamelen + 2];
-
- strcpy (buf, fname);
- buf[fnamelen - 1] = '?';
- e = glob (buf, 0, NULL, &gl);
- if (e == 0)
- {
- printf ("glob(\"%s\") succeeded\n", buf);
- retval = 1;
- }
- globfree (&gl);
-
- strcpy (buf, fname);
- buf[fnamelen] = '*';
- buf[fnamelen + 1] = '\0';
- e = glob (buf, 0, NULL, &gl);
- if (e == 0)
- {
- printf ("glob(\"%s\") succeeded\n", buf);
- retval = 1;
- }
- globfree (&gl);
-
- return retval;
-}
@@ -57,6 +57,9 @@
# define readdir(str) __readdir64 (str)
# define getpwnam_r(name, bufp, buf, len, res) \
__getpwnam_r (name, bufp, buf, len, res)
+# ifndef __lstat64
+# define __lstat64(fname, buf) __lxstat64 (_STAT_VER, fname, buf)
+# endif
# ifndef __stat64
# define __stat64(fname, buf) __xstat64 (_STAT_VER, fname, buf)
# endif
@@ -64,6 +67,7 @@
# define FLEXIBLE_ARRAY_MEMBER
#else /* !_LIBC */
# define __getlogin_r(buf, len) getlogin_r (buf, len)
+# define __lstat64(fname, buf) lstat (fname, buf)
# define __stat64(fname, buf) stat (fname, buf)
# define __fxstatat64(_, d, f, st, flag) fstatat (d, f, st, flag)
# define struct_stat64 struct stat
@@ -227,6 +231,18 @@ static int prefix_array (const char *prefix, char **array, size_t n) __THROWNL;
static int collated_compare (const void *, const void *) __THROWNL;
+/* Return true if FILENAME is a directory or a symbolic link to a directory.
+ Use FLAGS and PGLOB to resolve the filename. */
+static bool
+is_dir (char const *filename, int flags, glob_t const *pglob)
+{
+ struct stat st;
+ struct_stat64 st64;
+ return (__glibc_unlikely (flags & GLOB_ALTDIRFUNC)
+ ? pglob->gl_stat (filename, &st) == 0 && S_ISDIR (st.st_mode)
+ : __stat64 (filename, &st64) == 0 && S_ISDIR (st64.st_mode));
+}
+
/* Find the end of the sub-pattern in a brace expression. */
static const char *
next_brace_sub (const char *cp, int flags)
@@ -976,68 +992,53 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
can give the answer now. */
if (filename == NULL)
{
- struct stat st;
- struct_stat64 st64;
-
- /* Return the directory if we don't check for error or if it exists. */
- if ((flags & GLOB_NOCHECK)
- || (((__builtin_expect (flags & GLOB_ALTDIRFUNC, 0))
- ? ((*pglob->gl_stat) (dirname, &st) == 0
- && S_ISDIR (st.st_mode))
- : (__stat64 (dirname, &st64) == 0 && S_ISDIR (st64.st_mode)))))
- {
- size_t newcount = pglob->gl_pathc + pglob->gl_offs;
- char **new_gl_pathv;
+ size_t newcount = pglob->gl_pathc + pglob->gl_offs;
+ char **new_gl_pathv;
- if (newcount > SIZE_MAX / sizeof (char *) - 2)
- {
- nospace:
- free (pglob->gl_pathv);
- pglob->gl_pathv = NULL;
- pglob->gl_pathc = 0;
- retval = GLOB_NOSPACE;
- goto out;
- }
-
- new_gl_pathv = realloc (pglob->gl_pathv,
- (newcount + 2) * sizeof (char *));
- if (new_gl_pathv == NULL)
- goto nospace;
- pglob->gl_pathv = new_gl_pathv;
+ if (newcount > SIZE_MAX / sizeof (char *) - 2)
+ {
+ nospace:
+ free (pglob->gl_pathv);
+ pglob->gl_pathv = NULL;
+ pglob->gl_pathc = 0;
+ retval = GLOB_NOSPACE;
+ goto out;
+ }
- if (flags & GLOB_MARK)
- {
- char *p;
- pglob->gl_pathv[newcount] = malloc (dirlen + 2);
- if (pglob->gl_pathv[newcount] == NULL)
- goto nospace;
- p = mempcpy (pglob->gl_pathv[newcount], dirname, dirlen);
- p[0] = '/';
- p[1] = '\0';
- if (__glibc_unlikely (malloc_dirname))
- free (dirname);
- }
- else
- {
- if (__glibc_unlikely (malloc_dirname))
- pglob->gl_pathv[newcount] = dirname;
- else
- {
- pglob->gl_pathv[newcount] = strdup (dirname);
- if (pglob->gl_pathv[newcount] == NULL)
- goto nospace;
- }
- }
- pglob->gl_pathv[++newcount] = NULL;
- ++pglob->gl_pathc;
- pglob->gl_flags = flags;
+ new_gl_pathv = realloc (pglob->gl_pathv,
+ (newcount + 2) * sizeof (char *));
+ if (new_gl_pathv == NULL)
+ goto nospace;
+ pglob->gl_pathv = new_gl_pathv;
- return 0;
- }
+ if (flags & GLOB_MARK && is_dir (dirname, flags, pglob))
+ {
+ char *p;
+ pglob->gl_pathv[newcount] = malloc (dirlen + 2);
+ if (pglob->gl_pathv[newcount] == NULL)
+ goto nospace;
+ p = mempcpy (pglob->gl_pathv[newcount], dirname, dirlen);
+ p[0] = '/';
+ p[1] = '\0';
+ if (__glibc_unlikely (malloc_dirname))
+ free (dirname);
+ }
+ else
+ {
+ if (__glibc_unlikely (malloc_dirname))
+ pglob->gl_pathv[newcount] = dirname;
+ else
+ {
+ pglob->gl_pathv[newcount] = strdup (dirname);
+ if (pglob->gl_pathv[newcount] == NULL)
+ goto nospace;
+ }
+ }
+ pglob->gl_pathv[++newcount] = NULL;
+ ++pglob->gl_pathc;
+ pglob->gl_flags = flags;
- /* Not found. */
- retval = GLOB_NOMATCH;
- goto out;
+ return 0;
}
meta = __glob_pattern_type (dirname, !(flags & GLOB_NOESCAPE));
@@ -1245,15 +1246,9 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
{
/* Append slashes to directory names. */
size_t i;
- struct stat st;
- struct_stat64 st64;
for (i = oldcount; i < pglob->gl_pathc + pglob->gl_offs; ++i)
- if ((__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
- ? ((*pglob->gl_stat) (pglob->gl_pathv[i], &st) == 0
- && S_ISDIR (st.st_mode))
- : (__stat64 (pglob->gl_pathv[i], &st64) == 0
- && S_ISDIR (st64.st_mode))))
+ if (is_dir (pglob->gl_pathv[i], flags, pglob))
{
size_t len = strlen (pglob->gl_pathv[i]) + 2;
char *new = realloc (pglob->gl_pathv[i], len);
@@ -1359,56 +1354,6 @@ prefix_array (const char *dirname, char **array, size_t n)
return 0;
}
-/* We put this in a separate function mainly to allow the memory
- allocated with alloca to be recycled. */
-static int
-__attribute_noinline__
-link_stat (const char *dir, size_t dirlen, const char *fname,
- glob_t *pglob
-# if !defined _LIBC && !HAVE_FSTATAT
- , int flags
-# endif
- )
-{
- size_t fnamelen = strlen (fname);
- char *fullname = __alloca (dirlen + 1 + fnamelen + 1);
- struct stat st;
-
- mempcpy (mempcpy (mempcpy (fullname, dir, dirlen), "/", 1),
- fname, fnamelen + 1);
-
-# if !defined _LIBC && !HAVE_FSTATAT
- if (__builtin_expect ((flags & GLOB_ALTDIRFUNC) == 0, 1))
- {
- struct_stat64 st64;
- return __stat64 (fullname, &st64);
- }
-# endif
- return (*pglob->gl_stat) (fullname, &st);
-}
-
-/* Return true if DIR/FNAME exists. */
-static int
-link_exists_p (int dfd, const char *dir, size_t dirlen, const char *fname,
- glob_t *pglob, int flags)
-{
- int status;
-# if defined _LIBC || HAVE_FSTATAT
- if (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0))
- status = link_stat (dir, dirlen, fname, pglob);
- else
- {
- /* dfd cannot be -1 here, because dirfd never returns -1 on
- glibc, or on hosts that have fstatat. */
- struct_stat64 st64;
- status = __fxstatat64 (_STAT_VER, dfd, fname, &st64, 0);
- }
-# else
- status = link_stat (dir, dirlen, fname, pglob, flags);
-# endif
- return status == 0 || errno == EOVERFLOW;
-}
-
/* Like 'glob', but PATTERN is a final pathname component,
and matches are searched for in DIRECTORY.
The GLOB_NOSORT bit in FLAGS is ignored. No sorting is ever done.
@@ -1450,8 +1395,6 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
}
else if (meta == 0)
{
- /* Since we use the normal file functions we can also use stat()
- to verify the file is there. */
union
{
struct stat st;
@@ -1476,8 +1419,8 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
"/", 1),
pattern, patlen + 1);
if (((__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
- ? (*pglob->gl_stat) (fullname, &ust.st)
- : __stat64 (fullname, &ust.st64))
+ ? (*pglob->gl_lstat) (fullname, &ust.st)
+ : __lstat64 (fullname, &ust.st64))
== 0)
|| errno == EOVERFLOW)
/* We found this file to be existing. Now tell the rest
@@ -1501,8 +1444,6 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
}
else
{
- int dfd = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)
- ? -1 : dirfd ((DIR *) stream));
int fnm_flags = ((!(flags & GLOB_PERIOD) ? FNM_PERIOD : 0)
| ((flags & GLOB_NOESCAPE) ? FNM_NOESCAPE : 0));
flags |= GLOB_MAGCHAR;
@@ -1536,42 +1477,34 @@ glob_in_dir (const char *pattern, const char *directory, int flags,
if (fnmatch (pattern, d.name, fnm_flags) == 0)
{
- /* If the file we found is a symlink we have to
- make sure the target file exists. */
- dirent_type type = readdir_result_type (d);
- if (! (type == DT_LNK || type == DT_UNKNOWN)
- || link_exists_p (dfd, directory, dirlen, d.name,
- pglob, flags))
+ if (cur == names->count)
{
- if (cur == names->count)
- {
- struct globnames *newnames;
- size_t count = names->count * 2;
- size_t nameoff = offsetof (struct globnames, name);
- size_t size = FLEXSIZEOF (struct globnames, name,
- count * sizeof (char *));
- if ((SIZE_MAX - nameoff) / 2 / sizeof (char *)
- < names->count)
- goto memory_error;
- if (glob_use_alloca (alloca_used, size))
- newnames = names_alloca
- = alloca_account (size, alloca_used);
- else if ((newnames = malloc (size))
- == NULL)
- goto memory_error;
- newnames->count = count;
- newnames->next = names;
- names = newnames;
- cur = 0;
- }
- names->name[cur] = strdup (d.name);
- if (names->name[cur] == NULL)
+ struct globnames *newnames;
+ size_t count = names->count * 2;
+ size_t nameoff = offsetof (struct globnames, name);
+ size_t size = FLEXSIZEOF (struct globnames, name,
+ count * sizeof (char *));
+ if ((SIZE_MAX - nameoff) / 2 / sizeof (char *)
+ < names->count)
goto memory_error;
- ++cur;
- ++nfound;
- if (SIZE_MAX - pglob->gl_offs <= nfound)
+ if (glob_use_alloca (alloca_used, size))
+ newnames = names_alloca
+ = alloca_account (size, alloca_used);
+ else if ((newnames = malloc (size))
+ == NULL)
goto memory_error;
+ newnames->count = count;
+ newnames->next = names;
+ names = newnames;
+ cur = 0;
}
+ names->name[cur] = strdup (d.name);
+ if (names->name[cur] == NULL)
+ goto memory_error;
+ ++cur;
+ ++nfound;
+ if (SIZE_MAX - pglob->gl_offs <= nfound)
+ goto memory_error;
}
}
}
new file mode 100644
@@ -0,0 +1,132 @@
+/* Test glob danglin symlink match (BZ #866).
+ Copyright (C) 2017 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
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <unistd.h>
+#include <limits.h>
+#include <stddef.h>
+#include <glob.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include <support/check.h>
+#include <support/temp_file.h>
+
+static void do_prepare (int argc, char *argv[]);
+#define PREPARE do_prepare
+static int do_test (void);
+#include <support/test-driver.c>
+
+static void
+create_link (const char *base, const char *fname, char *linkname,
+ size_t linknamesize)
+{
+ int ntries = 0;
+ while (1)
+ {
+ snprintf (linkname, linknamesize, "%s/%s%02d", test_dir, base,
+ ntries);
+ if (symlink (fname, linkname) == 0)
+ break;
+ if (errno != EEXIST)
+ FAIL_EXIT1 ("symlink failed: %m");
+ if (ntries++ == 10)
+ FAIL_EXIT1 ("symlink failed with EEXIST too many times");
+ }
+ add_temp_file (linkname);
+}
+
+static char valid_link[PATH_MAX];
+static char dangling_link[PATH_MAX];
+static char dangling_dir[PATH_MAX];
+
+static void
+do_prepare (int argc, char *argv[])
+{
+ char *fname;
+
+ create_temp_file ("tst-glob_symlinks.", &fname);
+
+ /* Create a existing symlink. */
+ create_link ("valid-symlink-tst-glob_symlinks", fname, valid_link,
+ sizeof valid_link);
+
+ /* Create a dangling symlink to a file. */
+ int fd = create_temp_file ("dangling-tst-glob_file", &fname);
+ TEST_VERIFY_EXIT (close (fd) == 0);
+ /* It throws an warning at process end due 'add_temp_file' trying to
+ unlink it again. */
+ TEST_VERIFY_EXIT (unlink (fname) == 0);
+ create_link ("dangling-symlink-file-tst-glob", fname, dangling_link,
+ sizeof dangling_link);
+
+ /* Create a dangling symlink to a directory. */
+ char tmpdir[PATH_MAX];
+ snprintf (tmpdir, sizeof tmpdir, "%s/dangling-tst-glob_folder.XXXXXX",
+ test_dir);
+ TEST_VERIFY_EXIT (mkdtemp (tmpdir) != NULL);
+ create_link ("dangling-symlink-dir-tst-glob", tmpdir, dangling_dir,
+ sizeof dangling_dir);
+ TEST_VERIFY_EXIT (rmdir (tmpdir) == 0);
+}
+
+static int
+do_test (void)
+{
+ char buf[PATH_MAX];
+ glob_t gl;
+
+ TEST_VERIFY_EXIT (glob (valid_link, 0, NULL, &gl) == 0);
+ TEST_VERIFY_EXIT (gl.gl_pathc == 1);
+ TEST_VERIFY_EXIT (strcmp (gl.gl_pathv[0], valid_link) == 0);
+ globfree (&gl);
+
+ TEST_VERIFY_EXIT (glob (dangling_link, 0, NULL, &gl) == 0);
+ TEST_VERIFY_EXIT (gl.gl_pathc == 1);
+ TEST_VERIFY_EXIT (strcmp (gl.gl_pathv[0], dangling_link) == 0);
+ globfree (&gl);
+
+ TEST_VERIFY_EXIT (glob (dangling_dir, 0, NULL, &gl) == 0);
+ TEST_VERIFY_EXIT (gl.gl_pathc == 1);
+ TEST_VERIFY_EXIT (strcmp (gl.gl_pathv[0], dangling_dir) == 0);
+ globfree (&gl);
+
+ snprintf (buf, sizeof buf, "%s", dangling_link);
+ buf[strlen(buf) - 1] = '?';
+ TEST_VERIFY_EXIT (glob (buf, 0, NULL, &gl) == 0);
+ TEST_VERIFY_EXIT (gl.gl_pathc == 1);
+ TEST_VERIFY_EXIT (strcmp (gl.gl_pathv[0], dangling_link) == 0);
+ globfree (&gl);
+
+ /* glob should handle dangling symbol as normal file, so <file>? should
+ return an empty string. */
+ snprintf (buf, sizeof buf, "%s?", dangling_link);
+ TEST_VERIFY_EXIT (glob (buf, 0, NULL, &gl) != 0);
+ globfree (&gl);
+
+ snprintf (buf, sizeof buf, "%s*", dangling_link);
+ TEST_VERIFY_EXIT (glob (buf, 0, NULL, &gl) == 0);
+ TEST_VERIFY_EXIT (gl.gl_pathc == 1);
+ TEST_VERIFY_EXIT (strcmp (gl.gl_pathv[0], dangling_link) == 0);
+ globfree (&gl);
+
+ return 0;
+}