Petr Hracek <phra...@redhat.com> 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 <phra...@redhat.com> > 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 <phra...@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 >>>>> >>
