bug#10317: PING - bug#10317: patch to su: -l and -p should not be used together

[Jim Meyering]

Rocky Bernstein wrote:
> Any word on this?

Hi Rocky,

Sorry about the extended delay.
Changing su.c like this has really low priority.

I've looked at your patch and see that it changes the semantics
of su for those who use -l with (-m or -p).

Before your patch, -l would lead to simulate_login being set even
when -l was specified after an -m-or-p option.
Now, using e.g., "-l -m" evokes a warning but the -l is otherwise ignored.

Did you intend that?
Normally such a change would be mentioned in a ChangeLog entry or commit log.

I tried to use the su from coreutils (with or without your patch)
and found that it does not work when it attempts to authenticate.
E.g., it cannot su to any user on this Fedora 17 system.  If su
remains so broken that it does not work out-of-the-box on F17,
then it's not worth patching.  Remember that I wanted to remove su
from coreutils altogether, and only reluctantly agreed to consider
this change.  If someone who cares about su remaining in coreutils
wants to take responsibility for making it usable, that'd be great.
It may be as simple as importing a patch or two from Fedora or Suse.

If you'd like to pursue your change once su is restored to working
order, please justify or revert the semantic change in your patch,
starting from the change-set below, which includes the following changes:

  - remove some space-before-TAB in tests/Makefile.am
  - remove space-before-semicolon in su.c
  - split two longer-than-80-col lines
  - remove both \n and trailing "." from two new diagnostics
  - adjust NEWS

(some were caught by running "make syntax-check")


>From 4baf7f9558f45165e1250004ac710a62f4d505ff Mon Sep 17 00:00:00 2001
From: Rocky Bernstein <address@hidden>
Date: Fri, 4 May 2012 10:31:18 +0200
Subject: [PATCH] su: when -p and -l are used together, warn that they're
 exclusive

* src/su.c (main): FIXME-describe
* tests/su/p-and-l: New file.
* tests/Makefile.am (TESTS): Add it.
* doc/coreutils.texi: Document the change.
* NEWS (Changes in behavior): Mention this.
---
 NEWS               |  2 ++
 doc/coreutils.texi |  4 +++
 src/su.c           | 25 ++++++++++++++++--
 tests/Makefile.am  |  1 +
 tests/su/p-and-l   | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 104 insertions(+), 2 deletions(-)
 create mode 100644 tests/su/p-and-l

diff --git a/NEWS b/NEWS
index 1c00d96..cacbca3 100644
--- a/NEWS
+++ b/NEWS
@@ -24,6 +24,8 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   cp --attributes-only no longer truncates any existing destination file,
   allowing for more general copying of attributes from one file to another.

+  su -l -p gives a warning that these options are incompatible.
+

 * Noteworthy changes in release 8.16 (2012-03-26) [stable]

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 4fb52cb..a1d808c 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -15883,6 +15883,8 @@ su invocation
 directory.  Prepend @samp{-} to the shell's name, intended to make it
 read its login startup file(s).

+Note that this option and the next one @option{-m} are mutually exclusive.
+
 @item -m
 @itemx -p
 @itemx --preserve-environment
@@ -15901,6 +15903,8 @@ su invocation
 if that file does not exist.  Parts of what this option does can be
 overridden by @option{--login} and @option{--shell}.

+Note that this option and the previous one @option{-l} are mutually exclusive.
+
 @item -s @var{shell}
 @itemx address@hidden
 @opindex -s
diff --git a/src/su.c b/src/su.c
index bb54cc3..c049df9 100644
--- a/src/su.c
+++ b/src/su.c
@@ -399,6 +399,7 @@ main (int argc, char **argv)
   char *shell = NULL;
   struct passwd *pw;
   struct passwd pw_copy;
+  static bool seen_login_change_env = false;

   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
@@ -426,12 +427,32 @@ main (int argc, char **argv)
           break;

         case 'l':
-          simulate_login = true;
+          if (seen_login_change_env && !simulate_login)
+            {
+              error (0, 0,
+                     _("%s: already seen option --preserve-environment;"
+                       " --login ignored"), program_name);
+            }
+          else
+            {
+              simulate_login = true;
+              seen_login_change_env = true;
+            }
           break;

         case 'm':
         case 'p':
-          change_environment = false;
+          if (seen_login_change_env && change_environment)
+            {
+              error (0, 0,
+                     _("%s: already seen option --login;"
+                       " --preserve-environment ignored"), program_name);
+            }
+          else
+            {
+              change_environment = false;
+              seen_login_change_env = true;
+            }
           break;

         case 's':
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 72717e3..f2e06f4 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -517,6 +517,7 @@ TESTS =                                             \
   rmdir/fail-perm                              \
   rmdir/ignore                                 \
   rmdir/t-slash                                        \
+  su/p-and-l                                   \
   tail-2/assert-2                              \
   tail-2/big-4gb                               \
   tail-2/flush-initial                         \
diff --git a/tests/su/p-and-l b/tests/su/p-and-l
new file mode 100644
index 0000000..37fae6f
--- /dev/null
+++ b/tests/su/p-and-l
@@ -0,0 +1,74 @@
+#!/usr/bin/perl
+# Test whether options -p and -l give a warning.
+
+# Copyright (C) 2012 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+## The following may be helpful in debugging
+# use Enbugger; Enbugger->load_debugger('trepan');
+
+use strict; use warnings;
+(my $ME = $0) =~ s|.*/||;
+
+# Some older versions of Expect.pm (e.g. 1.07) lack the log_user method,
+# so check for that, too.
+eval { require Expect; Expect->require_version('1.11') };
+$@
+  and CuSkip::skip "$ME: this script requires Perl's Expect package >=1.11\n";
+
+{
+  my $fail = 0;
+  my @tests = (
+      ["su -l -p bogus",
+       qr/already seen option --login/],
+      ["su -p -l bogus",
+       qr/already seen option --preserve/],
+      );
+  foreach my $ary (@tests)
+    {
+      my ($cmd, $expect) = @$ary;
+      my $exp = new Expect;
+      $exp->log_user(0);
+      $exp->spawn("$cmd")
+        or (warn "$ME: cannot run `$cmd': $!\n"), $fail=1, next;
+      my $spawn_ok;
+      my @found = $exp->expect(1, 'Password: ');
+      unless ($found[3] =~ $expect) {
+         warn "$ME: $cmd did not get expected warning: $expect\n";
+         $fail = 1;
+      }
+      $exp->hard_close();
+    }
+  @tests = (
+      "su -l bogus",
+      "su -p bogus",
+      "su bogus",
+      );
+  foreach my $cmd (@tests)
+    {
+      my $exp = new Expect;
+      $exp->log_user(0);
+      $exp->spawn("$cmd")
+        or (warn "$ME: cannot run `$cmd': $!\n"), $fail=1, next;
+      my $spawn_ok;
+      my @found = $exp->expect(1, 'Password: ');
+      if ($found[3] =~ qr/already seen option/) {
+         warn "$ME: $cmd should not get already-seen option warning\n";
+         $fail = 1;
+      }
+      $exp->hard_close();
+    }
+  exit $fail
+}
--
1.7.10.1.456.g16798d0