[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#13588: Pax hangs in case big UID
From: |
Jack Kelly |
Subject: |
bug#13588: Pax hangs in case big UID |
Date: |
Fri, 22 Mar 2013 12:39:46 +1100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) |
Petr Hracek <address@hidden> writes:
> Hello Jack and Stefano,
>
> Bellow is corrected patch for automake.
> Jack thank you for corrections. Now the patch looks like better.
Yes, it looks a lot better. I have more thoughts, if that's ok:
+ # Maximum allowed UID in case ustar format is 2097151
Put a link to
http://lists.gnu.org/archive/html/bug-automake/2011-11/msg00014.html so
it's clear where the number came from.
I'd do it like this (I've also removed the variable assignments for
$user_id and $group_id):
m4_if($1, [ustar],
[AC_CHECK_PROG([am_prog_have_id], [id], [yes], [no])
if test x"$am_prog_have_id" = x"yes"; then
# POSIX 1988 "ustar" format is defined with *fixed size* fields. There
# is notably a 21 bits limit (2097151) for the uid and the gid.
# http://lists.gnu.org/archive/html/bug-automake/2011-11/msg00014.html so
am_ustar_max_id=2097151
...
if test $? -eq 0 -a `id -u` -gt $am_ustar_max_id; then
...
])
What should the user be told? It's not really their fault that the
developer specified ustar-format archives, so perhaps the error message
should ask the user to run configure with a lower UID or ask the user to
ask upstream to stop using the ustar option.
-- Jack
> From 98a64a309a0f7271d2772dd63e45e43b1163c315 Mon Sep 17 00:00:00 2001
> From: Petr Hracek <address@hidden>
> Date: Thu, 21 Mar 2013 13:27:39 +0100
> Subject: [PATCH] maint: pax hangs in case big UID
>
> See automake bug #13588
>
> * m4/tar.m4: check for ustar V7 archive format. Maximum value for user
> or group ID
> is limited to 2097151. Thanks to Jack Kelly for patch corrections
> ---
> m4/tar.m4 | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/m4/tar.m4 b/m4/tar.m4
> index ec8c83e..9b1d206 100644
> --- a/m4/tar.m4
> +++ b/m4/tar.m4
> @@ -81,6 +81,28 @@ do
> AM_RUN_LOG([tardir=conftest.dir && eval $am__tar_ >conftest.tar])
> rm -rf conftest.dir
> if test -s conftest.tar; then
> + 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`
> + # Maximum allowed UID in case ustar format is 2097151
> + if test $? -eq 0 -a $user_id -gt 2097151; then
> + AC_MSG_RESULT([no])
> + AC_MSG_ERROR([the uid is too large for ustar-format
> tarfiles. Change format in configure.ac],[2])
> + else
> + AC_MSG_RESULT([yes])
> + fi
> + AC_MSG_CHECKING([if gid is small enough for ustar])
> + group_id=`id -g`
> + # Maximum allowed GID in case ustar format is 2097151
> + if test $? -eq 0 -a $group_id -gt 2097151; then
> + AC_MSG_RESULT([no])
> + AC_MSG_ERROR([the gid is too large for ustar-format
> tarfiles. Change format in configure.ac],[2])
> + else
> + AC_MSG_RESULT([yes])
> + fi
> + fi])
> AM_RUN_LOG([$am__untar <conftest.tar])
> grep GrepMe conftest.dir/file >/dev/null 2>&1 && break
> fi
> --
> 1.8.1.4
>
>
> On 03/20/2013 01:02 PM, Petr Hracek wrote:
>> 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 <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
>>>>>
>>