[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] maint.mk: add syntax-check to avoid char[PATH_MAX]
From: |
Jim Meyering |
Subject: |
Re: [PATCH] maint.mk: add syntax-check to avoid char[PATH_MAX] |
Date: |
Thu, 23 Jun 2011 11:02:40 +0200 |
Eric Blake wrote:
> POSIX allows PATH_MAX to be undefined. And even if you use the
> gnulib pathmax module, where "pathmax.h" guarantees a definition,
> the definition might not be constant or might be so large as to
> be wasteful or cause stack overflows. PATH_MAX should only be
> used as a limit or hueristic, not an array size.
>
> * top/maint.mk (sc_prohibit_path_max_array): New rule.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>
> Within gnulib itself, there are some exceptions: lib/tmpname.c
> and lib/stat.c use char[PATH_MAX] but only on mingw, where we
> know the value is constant and small. But this new rule helped
> catch some violations in libvirt.
>
> ChangeLog | 3 +++
> top/maint.mk | 9 +++++++++
> 2 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index b4b82fa..9648603 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,8 @@
> 2011-06-22 Eric Blake <address@hidden>
>
> + maint.mk: add syntax-check to avoid char[PATH_MAX]
> + * top/maint.mk (sc_prohibit_path_max_array): New rule.
> +
> stat: be robust to PATH_MAX definition
> * lib/stat.c (rpl_stat): Require reasonable PATH_MAX.
> * modules/stat (Depends-on): Add verify.
> diff --git a/top/maint.mk b/top/maint.mk
> index 6f6b8be..4408a4e 100644
> --- a/top/maint.mk
> +++ b/top/maint.mk
> @@ -1106,6 +1106,7 @@ sc_copyright_check:
> # the other init.sh-using tests also get it right.
> _hv_file ?= $(srcdir)/tests/help-version
> _hv_regex_weak ?= ^ *\. .*/init\.sh"
> +# Fix syntax-highlighters "
> _hv_regex_strong ?= ^ *\. "\$${srcdir=\.}/init\.sh"
> sc_cross_check_PATH_usage_in_tests:
> @if test -f $(_hv_file); then \
> @@ -1133,6 +1134,14 @@ sc_Wundef_boolean:
> halt='Use 0 or 1 for macro values' \
> $(_sc_search_regexp)
>
> +# Even if you use pathmax.h to guarantee that PATH_MAX is defined, it might
> +# not be constant. In general, use PATH_MAX as a limit, not an array
> +# allocation bound.
> +sc_prohibit_path_max_array:
> + @prohibit='\[PATH''_MAX' \
> + halt='Avoid arrays of size PATH_MAX' \
> + $(_sc_search_regexp)
I like it.
As you and Paul discussed, some projects may well
want to disable the test, but that is easy enough.
However, I would suggest a more permissive regexp, e.g.,
@prohibit='\[[^]]*PATH''_MAX' \
Then it will catch these three in git.git that
the simpler regexp would have missed:
builtin/pack-objects.c: char line[40 + 1 + PATH_MAX + 2];
builtin/receive-pack.c: static char buf[sizeof(commands->old_sha1) * 2
+ PATH_MAX + 4];
fast-import.c: char message[2 * PATH_MAX];
I noticed that idutils has many uses like the following,
which are inappropriate when PATH_MAX is large:
src/fid.c: char *file_name_buf = alloca (PATH_MAX);
src/fnid.c: char *file_name = alloca (PATH_MAX);
We could detect those, too:
@prohibit='(\balloca *\([^)]*|\[[^]]*)PATH''_MAX' \
but that would require a different diagnostic, since there
is no "array" involved, so it may be better to leave that for
a separate stack-size-related test.