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

[Rocky Bernstein]

On Fri, May 4, 2012 at 4:49 AM, Jim Meyering <address@hidden> wrote:

> 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?
>

I never had a strong feeling on it to the extent that ambiguous behavior
should be preserved. However much more important is issuing a warning says
what's done whatever it is and currently, that is not done.  I am sensing
(but am not completely sure that)  the preferred behavior is to keep
compatible behavior whatever the current behavior is? May as well get this
cleared up before another 1/3 of a year goes by.

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,
>


Actually, at this point I am loosing interest. You have created a somewhat
unfriendly environment here to work in. I suggest, but leave up to you,
whether to just document the behavior as it is leaving the code exactly as
it is.



> 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
>

Again, if you or anyone reading this can't be bothered to do this, I will
also put this on my  "low priority" list. You have in mind what you want to
see and I've been getting this information in drips and drabs which is
annoying.

It's now about 5 months since I sent this and only now have you even
attempted to try the patch. (The last correspondence you incorrectly made a
denigrating assumption which just added gratuitous delay.)  Most of the
stylistic things mentioned here could have been mentioned a couple of
months ago.

When I say something is "low priority" it generally means that eventually
when I get around to it I will deal with it rather than kick it down the
can again.


>
> (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
>