bug#13588: Pax hangs in case big UID

[Jack Kelly]

Hi Petr, I have a couple of observations:

- AC_MSG_ERROR is going to stop the configure anyway, so you don't need
  exit 1.

- I'd suggest the following messages for your AC_MSG_ERRORS:

  "the uid is too large for ustar-format tarfiles. Change format in
  configure.ac"

  "the gid is too large for ustar-format tarfiles. Change format in
  configure.ac"

- Is there a reason you've gone with "test -ge 2097152" instead of "test
  -gt 2097151"?

- Test for $1 = ustar first, so you only AC_CHECK_PROG for id when
  needed.

- You could merge some of those nested tests:

if test $? -eq 0 -a $user_id -gt 2097151; then
  AC_MSG_ERROR([...])
fi

- That argument can be conditionally expanded at m4 time, so it'll be a
  bit faster for end users to use an m4 conditional here.

- In fact, you might want to structure it like this:

m4_if($1, [ustar],
[AC_CHECK_PROG([am_prog_have_id], [id], [yes], [no])
if test x"$am_prog_have_id" = x"yes"; then
  AC_MSG_CHECKING([if uid is small enough for ustar])
  user_id=`id -u`
  if test $? -eq 0 -a $user_id -gt 2097151; then
    AC_MSG_RESULT([yes])
  else
    AC_MSG_RESULT([no])
    AC_MSG_ERROR([...])
  fi
  AC_MSG_CHECKING([if gid is small enough for ustar])
  group_id=`id -g`
  if test $? -eq 0 -a $group_id -gt 2097151; then
    AC_MSG_RESULT([yes])
  else
    AC_MSG_RESULT([no])
    AC_MSG_ERROR([...])
  fi
fi])

But that's just my opinion, and Stefano's the one who vets the patches
around here.

Stefano: is this the sort of thing that should have
AC_MSG_CHECKING/AC_MSG_RESULT pairs? Also, have I got the m4 quoting
right?

-- Jack

Petr Hracek <address@hidden> writes:
> Hello Stefano,
> diff --git a/m4/tar.m4 b/m4/tar.m4
> index ec8c83e..87477f1 100644
> --- a/m4/tar.m4
> +++ b/m4/tar.m4
> @@ -81,6 +81,27 @@ do
>    AM_RUN_LOG([tardir=conftest.dir && eval $am__tar_ >conftest.tar])
>    rm -rf conftest.dir
>    if test -s conftest.tar; then
> +    AC_CHECK_PROG([ID_TEST], id, [yes], [no])
> +    if test x"$ID_TEST" = x"yes"; then
> +      if test x"$1" = x"ustar" ; then
> +         user_id=`id -u`
> +         if test $? -eq 0; then
> +           # Maximum allowed UID in case ustar format is 2097151.
> +           if test $user_id -ge 2097152; then
> +             AC_MSG_ERROR([The uid is big and not allowed in case of
> ustar format. Change format in configure.ac],[2])
> +             exit 1
> +           fi
> +         fi
> +         group_id=`id -g`
> +         if test $? -eq 0; then
> +           # Maximum allowed GID in case ustar format is 2097151
> +           if test $group_id -ge 2097152; then
> +             AC_MSG_ERROR([The gid is big and not allowed in case of
> ustar format. Change format in configure.ac],[2])
> +             exit 1
> +           fi
> +         fi
> +      fi
> +    fi
>      AM_RUN_LOG([$am__untar <conftest.tar])
>      grep GrepMe conftest.dir/file >/dev/null 2>&1 && break
>    fi
>
> I would like to help you so that all issues like style, commit
> message,will be properly set.
> If you agree I will commit that patch in accordance GNU coding style
> and HACKING file.
>
> Best regards
> Petr
> On 02/05/2013 02:51 PM, Stefano Lattarini wrote:
>> Hi Peter and Petr (no typo :-)
>>
>> FYI, this patch is on my radar, and I intend to consider it (and
>> likely integrate and adjusted version of it) in the upcoming minor
>> version of Automake (1.13.2).
>>
>> If you want to speed things up a bit and make the integration
>> work easier to us, you can present the patch as the output of
>> "git format-patch" rather than of "git diff", and add a clear
>> git commit message that explains the rationale and motivation
>> for the change.  That would be appreciated.
>>
>> On 02/05/2013 01:42 PM, Petr Hracek wrote:
>>> Hello Peter,
>>>
>>> no problem, I will wait.
>>> AC_SUBST is used for one place instance of the variable
>>> so that we will modify (in future) only one row instead of several.
>>>
>> I don't understand this rationale; and I agree with Peter that the
>> AC_SUBST call on AM_BIG_ID is unneeded; this should be just a shell
>> variable (and since it is used only internally by the automake
>> generated code, it should likely be renamed to something like
>> 'am_big_id', or even '_am_big_id'.
>>
>> There are other issues too, but I'll get to them when I'll do a proper
>> review.
>>
>>> I hope that this is not too much general.
>>>
>>> BR
>>> Petr
>> Thanks,
>>    Stefano
>>