bug-gnulib
[Top][All Lists]
Advanced

[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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]