Re: Configure should failed in case format ustar and big UID are used

[Stefano Lattarini]

On 02/20/2013 02:40 PM, Petr Hracek wrote:
> Hi,
> 
Hi Petr, sorry for the delay.

> after reading of ustar format 
> (http://www.gnu.org/software/tar/manual/html_section/Formats.html)
> I have made patch for m4/tar.m4 file against upstream:
> In case that ustar format is used and uid or gid is bigger than 2^21
> than configure will always failed
>
Yes, you have already reported that (offering a fix):

   <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13588>

As I said, I will integrate it before the next beta for 1.13.2, and will
get back to you for ACK and testing.  I can't promise I will do so
quickly, but I will definitely do it.  Please be patient (and again,
sorry for the delay).

Now, some miscellaneous nits about your patch, which you might want to
take into account in case you need to send other patches in the future
(which will make the review and integration of your patches faster and
more efficient):

> Patch is:
> diff --git a/m4/tar.m4 b/m4/tar.m4
> index ec8c83e..1eca282 100644
> --- a/m4/tar.m4
> +++ b/m4/tar.m4
>
We actually prefer patches formatted with "git-format", that are easier
to apply with git-provided tools.

Also, when preparing a commit, you should write a proper commit message
explaining what changes it introduces, and for what reasons; this should
be complemented by a ChangeLog entry in the GNU style.  See the file
HACKING (section "Writing a good commit message") in the top-level
directory of the Automake source tree for more information.

> @@ -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])
>
No need to check this (see below).  Also, using 'ID_TEST' as a variable,
you are invading the user's name space; since this is a variable for
internal use, a name like '_am_have_id' would have been more appropriate.

> +    if test x"$ID_TEST" = x"yes"; then
>
This conditional should be dropped as well (see below),

> +      if test x"$1" = x"ustar" ; then
>
This should be an m4-time conditional, not a shell-time one, to avoid
emitting useless code in the generated configure script.

> +         user_id=`id -u`
> +         if test $? -eq 0; then
>
This is better:

    if user_id=`id -u 2>/dev/null` && test $user_id -ge 0; then ...

Also, we should avoid invading the user's namespace -- what if he's
using a variable named 'user_uid' in other parts of his configure
script?  A variable name like '_am_uid' would be more appropriate.

> +           # 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])
>
This change would prevent an user with a "high" ID to install a
package that he can in all likelihood build and run without problems,
only because he *might* see some failures if he tried to run
"make dist" -- a target this reserved for developers and maintainers
anyway!  This is unacceptable.  The right thing to do if we find out
that the user ID is too large is simply to skip the later test on pax
that might cause configure to hand.  If configure is not able to find
any appropriate packaging tool, the user won't be able to build a
distribution tarball from his system -- no big deal.  OTOH, having
configure to hang during a test is a serious problem, and we need to
prevent that from happening.

Also, as a further aside, notice that a basic tenet of the autotools
philosophy is that the the final users should not be required to have
*any* of the autotools installed in ordter to configure, build or
install a distributed package created with the autotools; suggesting
the user to edit configure.ac and to re-run the autotools to fix a
configure-time problem is in general unacceptable.

> +             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
>
The same issues pointed out above apply to this hunk as well.

> +      fi
> +    fi
>      AM_RUN_LOG([$am__untar <conftest.tar])
>      grep GrepMe conftest.dir/file >/dev/null 2>&1 && break
>    fi
> 
> Please review it and let me now whether something is wrong.
> If something will be wrong I will make a correction.
> Tests where performed as on big uid as on big gid and both.
>
> I have been looking for making test in that case, but found
> that useradd binary should be called and I think that we do
> not what to do that.
> 
I agree a test case for this would be too demanding.  I will
ask you to test my patch in your production environment before
applying it, to ensure it actually fix the issue under discussion.

Thanks,
  Stefano