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 <meyer...@redhat.com> 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 <meyer...@redhat.com> + 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
