GNU bug report logs - #13588
Pax hangs in case big UID

Previous Next

Full log

View this message in rfc822 format

From: Petr Hracek <phracek <at> redhat.com>
To: Jack Kelly <jack <at> jackkelly.name>
Cc: 13588 <at> debbugs.gnu.org, Stefano Lattarini <stefano.lattarini <at> gmail.com>
Subject: bug#13588: Pax hangs in case big UID
Date: Wed, 20 Mar 2013 13:02:36 +0100

Hello Jack,

that's sound better than my proposed patched.
It's shorter and  more understandable.

Yes you are right that user should be informed for testing ustar format.

Best regards
Petr


On 03/20/2013 12:55 PM, Jack Kelly wrote:
> 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 <phracek <at> redhat.com> 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
>>>

-- 
S pozdravem / Best regards

Petr Hracek

Red Hat Czech s.r.o.
BaseOS Core Services Brno

Email: phracek <at> redhat.com
Web: www.cz.redhat.com

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.