[Pkg-shadow-devel] Bug#276419: Overflated severity?

[Helmut Waitzmann (Debian Bug Tracking System)]

--=-=-=
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable

Christian Perrier <bubulle@debian.org> writes:

>I'm afraid I don't really see the rationale behind the severity of
>this bug report.
>
>Does it really deserve the "important" severity because it "has a
>major effect on the usability of a package, without rendering it
>completely unusable to everyone"?
>
>My current opinion is that it does not fit this definition which makes
>me think that it should have its severity lowered to "normal".
>
>After all, all common use of su are not affected by this bug, which is
>triggered in a rather specific situation.

Is this situation specific enough?

$ find some files to be archived -print0 | xargs -0 -e -r -- \
  sh -c 'exec su -- - archiver ${1+"$@"} < /dev/tty' sh

(The user "archiver" has got a special "shell": a program which expects
file names of files to be archived as positional parameters).

Nicolas Fran=E7ois' patch solves the commandline problem.

A second problem, which is related to the first (erroneous processing
of the shell's arguments), but afaics is not solved by his patch:

I like to use su when I'm going to change and test the configuration of
my login shell.  To see, what's going on, when doing a login I'd like to
start the command

$ su -- - "$LOGNAME" -x

in order to get a login shell that echoes the commands it's going to
execute.

However, something is going wrong:

$ su -- - "$LOGNAME" -x
Password:=20
-su: option `-c' requires an argument

According to the manual page su(1) and to the Linux Standard Base
Specification 1.3 and 2.0.1, su will process its option arguments and
pass all ARGS following the username to the shell to be started.  In this
case the ARGS would be the single argument "-x":  The login shell will be
invoked with the positional arguments vector { "-su", "-x", NULL }.

su supplies the arguments vector { "-su", "-c", "-x", NULL } to the shell
to be invoked and breaks those requirements (which is afair a severe
violation of the Debian policy).

The Problem lies at lines 209 to 210 of Nicolas Fran=E7ois' patched version
of su.c:

  if (command || additional_args)
    args[argno++] =3D "-c";

This patch (applied after Nicolas Fran=E7ois' patch) should correct that:


--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment; filename=commandline.2.patch
Content-Description: patch for su.c correcting the commandline processing

--- su.c.orig	Tue Mar 29 06:26:15 2005
+++ su.c	Wed Mar 30 21:31:22 2005
@@ -172,8 +172,9 @@
 {
   int n = 0;
 
-  for (n = 0; *arr; ++arr)
-    ++n;
+  if (arr)
+    for (n = 0; *arr; ++arr)
+      ++n;
   return n;
 }
 
@@ -184,11 +185,24 @@
   const char **args;
   int argno = 1;
 
-  if (additional_args)
-    args = (const char **) xmalloc (sizeof (char *)
-                                    * (10 + elements (additional_args)));
-  else
-    args = (const char **) xmalloc (sizeof (char *) * 10);
+  /* Allocate 2 up to 4 more slots for the argument vector than there are
+     additional args:
+     1 for args[0],
+     2 (optional) for a commandline, preceded with "-c",
+     1 for the args[]-terminating NULL entry.
+   */
+  args = (const char **)
+    xmalloc (
+      sizeof (char *) *
+      (
+	1 /* args[0] */
+	+
+	(command ? 2 : 0) /* "-c" "commandline", if supplied */
+	+
+	elements (additional_args) /* number of additional args: */
+	+
+	1 /* the terminating NULL entry */
+      ));
 
   if (login)
     {
@@ -206,9 +220,14 @@
     }
   else
     args[0] = Basename(shell);
-  if (command || additional_args)
-    args[argno++] = "-c";
-  if (command && command[0]) args[argno++] = command;
+  if (command)
+    { /* A command option "-c" or "--command" has been supplied.  Insert
+       * "-c" and its option argument, i.e. the commandline, into the
+       * argument vector.
+       */
+      args[argno++] = "-c";
+      args[argno++] = command;
+    }
   if (additional_args)
     for (; *additional_args; ++additional_args)
       args[argno++] = *additional_args;

--=-=-=


There are some minor corrections included:

* Compute the exact size of the argument vector, so don't allocate a
  longer one than is necessary.

* The function 'elements' doesn't dereference a NULL pointer argument.


I'd like to see the function 'elements' returning a value of type size_t
rather than of type int.

The following patch reflects this.


--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment; filename=size_t.patch
Content-Description: a patch for su.c: elements() returns size_t

--- su.c.orig	Wed Mar 30 21:31:22 2005
+++ su.c	Wed Mar 30 21:30:43 2005
@@ -167,10 +167,10 @@
 }
 
 /* borrowed from GNU sh-utils' "su.c" */
-static int
+static size_t
 elements (char **arr)
 {
-  int n = 0;
+  size_t n = 0;
 
   if (arr)
     for (n = 0; *arr; ++arr)

--=-=-=
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable


su.c does include <sys/types.h>.  It is necessary, to include <stddef.h>
in order to have size_t defined?  If the answer is "yes", then the last
patch won't be enough.  If one inserts a '#include <stddef.h>', what
makefiles, autoconffiles, etc. will have to be changed, too?

If you like, apply this patch after the one above.


As I couldn't compile neither Nicolas Fran=E7ois' nor the following patches
(Debian Woody lacks automake1.7), this patches are untested.  Would you
please have a look at them?
--=20
Wenn Sie mir E-Mail schreiben, stellen |  When writing me e-mail, please
Sie bitte vor meine E-Mail-Adresse     |  precede my e-mail address with
meinen Vor- und Nachnamen, etwa so:    |  my full name, like
Helmut Waitzmann <xxx@example.net>, (Helmut Waitzmann) xxx@example.net

--=-=-=--