Re: [PATCH] canonicalize: add support for not resolving symlinks

[Jim Meyering]

Pádraig Brady wrote:
> On 12/30/2011 01:49 PM, Eric Blake wrote:
>> On 12/30/2011 04:04 AM, Jim Meyering wrote:
>>>> -          if (lstat (rname, &st) != 0)
>>>> +          if ((logical?stat:lstat) (rname, &st) != 0)
>>>
>>> Please add spaces around operators:
>>>
>>>              if ((logical ? stat : lstat) (rname, &st) != 0)
>>
>> Better yet, don't write this as a function pointer conditional, as the
>> gnulib replacement for stat on some platforms has to be a function-like
>> macro (no thanks to 'struct stat').  Using a function pointer
>> conditional risks missing the gnulib macro for stat, and calling the
>> underlying system stat() instead of the intended rpl_stat(), for broken
>> behavior.  You have to use:
>>
>> if (logical ? stat (rname, &st) : lstat (rname, &st)) != 0)
>>
>
> Ouch. Good catch.
> I'll need to fix in a follow up patch.
> I'll need to adjust such uses in coreutils too.

Oh!  I'd just finished checking gnulib for other instances,
then, after seeing your message, did this in coreutils:

  $ git grep -E '\<l?stat *: *l?stat\>'
  src/ln.c:          (logical?stat:lstat) (source, &source_stats)
  src/stat.c:     (follow_links?stat:lstat) (filename, &statbug)
  src/touch.c:      /* Don't use (no_dereference?lstat:stat) (args), since stat

Here's a proposed addition to gnulib's maint.mk:

>From 24b71909a0b4022aeeea7fa74f250afa243e8d7a Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 30 Dec 2011 16:06:33 +0100
Subject: [PATCH] maint.mk: add a syntax check for subtle stat/lstat usage
 problem

* top/maint.mk (sc_stat_lstat_without_open_paren): New rule.
Motivated by this exchange:
http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/29491/focus=29496
---
 ChangeLog    |    5 +++++
 top/maint.mk |   10 ++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e61be47..e2e1567 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2011-12-30  Jim Meyering  <address@hidden>

+       maint.mk: add a syntax check for subtle stat/lstat usage problem
+       * top/maint.mk (sc_stat_lstat_without_open_paren): New rule.
+       Motivated by this exchange:
+       http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/29491/focus=29496
+
        gitlog-to-changelog: remove a little duplication
        * build-aux/gitlog-to-changelog (main): Grep @lines once,
        rather than twice.
diff --git a/top/maint.mk b/top/maint.mk
index e4efb5f..9386a61 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -1185,6 +1185,16 @@ sc_prohibit_path_max_allocation:
        halt='Avoid stack allocations of size PATH_MAX'                 \
          $(_sc_search_regexp)

+# Use stat or lstat only followed by an opening parenthesis, i.e.,
+# Don't do this: (cond ? lstat : stat) (....
+# Do this instead: (cond ? lstat (...) : stat (...))
+# Otherwise, they may fail to resolve (via a macro) to a desired
+# replacement function.
+sc_stat_lstat_without_open_paren:
+       @prohibit='\<l?stat *: *l?stat\>'
+       halt='don'\''t use stat or lstat without an open parenthesis'   \
+         $(_sc_search_regexp)
+
 sc_vulnerable_makefile_CVE-2009-4029:
        @prohibit='perm -777 -exec chmod a\+rwx|chmod 777 \$$\(distdir\)' \
        in_files=$$(find $(srcdir) -name Makefile.in)                   \
--
1.7.8.1.391.g2c2ad