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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 10317 in the body.
You can then email your comments to 10317 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: bug-coreutils <at> gnu.org
Subject: patch to su: -l and -p should not be used together
Date: Sat, 17 Dec 2011 15:53:07 -0500

[Message part 1 (text/plain, inline)]

Hi -

I noticed that -m (-p, --preserve-environment) and -l (- --login) can't be
used together.

Attached is a patch to give a warning if they are and to indicate which
option is used. In the code, I opted for the earlier option ignoring the
later option. In the code --login has precedence over
--preserve-environment.

[Message part 2 (text/html, inline)]

[su-l-vs-p.diff (text/x-patch, attachment)]

Message #8 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: 10317 <at> debbugs.gnu.org
Subject: PING - bug#10317: patch to su: -l and -p should not be used together
Date: Sun, 12 Feb 2012 20:59:35 -0500

[Message part 1 (text/plain, inline)]

Hi -

It's been a couple of months since I first sent this was sent without nary
an ack. Comments?

[Message part 2 (text/html, inline)]

Message #11 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rocky Bernstein <rocky <at> gnu.org>
Cc: 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Mon, 13 Feb 2012 10:19:44 +0100

Rocky Bernstein wrote:
> It's been a couple of months since I first sent this was sent without nary
> an ack. Comments?

Hi Rocky,

su is barely on life support in coreutils.
Meaning that it's no longer really maintained.
We stopped installing it (by default) back in the 2007-2008
time frame (6.9.90, 7.0).  We nearly removed it altogether.

Do you build/install coreutils yourself, or use it via a distribution?
If the latter, which distribution?

I don't remember looking carefully at your patch before,
but glanced through it just now.  Here are some suggestions
if you'd like to pursue it:

  - use "error (0, 0, ...", not fprintf (
  - indent with spaces, not TABs
  - use gnu indentation/formatting style (esp. wrt braces)
  - mention the change in NEWS
  - for a behavior change like this, we would like a test case

Message #14 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Mon, 13 Feb 2012 21:29:03 -0500

[Message part 1 (text/plain, inline)]

On Mon, Feb 13, 2012 at 4:19 AM, Jim Meyering <jim <at> meyering.net> wrote:

> Rocky Bernstein wrote:
> > It's been a couple of months since I first sent this was sent without
> nary
> > an ack. Comments?
>
> Hi Rocky,
>
> su is barely on life support in coreutils.
> Meaning that it's no longer really maintained.
> We stopped installing it (by default) back in the 2007-2008
> time frame (6.9.90, 7.0).  We nearly removed it altogether.
>
> Do you build/install coreutils yourself, or use it via a distribution?
> If the latter, which distribution?
>

CenOS 6.2 which seems to use coreutils 8.4

>
> I don't remember looking carefully at your patch before,
> but glanced through it just now.  Here are some suggestions
> if you'd like to pursue it:
>
>  - use "error (0, 0, ...", not fprintf (
>  - indent with spaces, not TABs
>  - use gnu indentation/formatting style (esp. wrt braces)
>  - mention the change in NEWS
>  - for a behavior change like this, we would like a test case
>

The above seems all reasonable and easily doable. But are you suggesting is
even after getting this into the next coreutils, no OS is likely to use it?

The basic idea is that a colleague and I were confused by the fact that -l
and -p conflict although this was not apparent in the documentation. I
wound up downloading the source code to understand fully.

So something even as simple as mentioning this in doc/coreutils.texi would
have been helpful (which is the first part of that patch).

Going further I realized that it is probably erroneous to supply both -l -p
and that can be easily warned.

But if there is a better place such to report such problems or get code
improved, let me know! Thanks.

[Message part 2 (text/html, inline)]

Message #17 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rocky Bernstein <rocky <at> gnu.org>
Cc: 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Tue, 14 Feb 2012 11:49:16 +0100

Rocky Bernstein wrote:

> On Mon, Feb 13, 2012 at 4:19 AM, Jim Meyering <jim <at> meyering.net> wrote:
>
>     Rocky Bernstein wrote:
>     > It's been a couple of months since I first sent this was sent without nary
>     > an ack. Comments?
>
>     Hi Rocky,
>
>     su is barely on life support in coreutils.
>     Meaning that it's no longer really maintained.
>     We stopped installing it (by default) back in the 2007-2008
>     time frame (6.9.90, 7.0).  We nearly removed it altogether.
>
>     Do you build/install coreutils yourself, or use it via a distribution?
>     If the latter, which distribution?
>
> CenOS 6.2 which seems to use coreutils 8.4
>
>     I don't remember looking carefully at your patch before,
>     but glanced through it just now.  Here are some suggestions
>     if you'd like to pursue it:
>
>      - use "error (0, 0, ...", not fprintf (
>      - indent with spaces, not TABs
>      - use gnu indentation/formatting style (esp. wrt braces)
>      - mention the change in NEWS
>      - for a behavior change like this, we would like a test case
>
> The above seems all reasonable and easily doable. But are you suggesting is even
> after getting this into the next coreutils, no OS is likely to use it?
>
> The basic idea is that a colleague and I were confused by the fact that -l and
> -p conflict although this was not apparent in the documentation. I wound up
> downloading the source code to understand fully.
>
> So something even as simple as mentioning this in doc/coreutils.texi would have
> been helpful (which is the first part of that patch).
>
> Going further I realized that it is probably erroneous to supply both -l -p and
> that can be easily warned.
>
> But if there is a better place such to report such problems or get code
> improved, let me know! Thanks.

I see that Fedora still uses su from coreutils, too,
so this is a worthwhile change.

Message #20 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: "Voelker, Bernhard" <bernhard.voelker <at> siemens-enterprise.com>
To: Jim Meyering <jim <at> meyering.net>, Rocky Bernstein <rocky <at> gnu.org>
Cc: "10317 <at> debbugs.gnu.org" <10317 <at> debbugs.gnu.org>
Subject: RE: bug#10317: PING - bug#10317: patch to su: -l and -p should not
	be	used together
Date: Tue, 14 Feb 2012 12:31:07 +0100

Jim Meyering wrote:

> I see that Fedora still uses su from coreutils, too,
> so this is a worthwhile change.

So does OpenSuSE.

As in coreutils.texi, -l and -p can be used together,
although it's not very clear what will happen:

  @item -m
  @itemx -p
  @itemx --preserve-environment
  ...
  Parts of what this option does can be
  overridden by @option{--login} and @option{--shell}.

Have a nice day,
Berny

Message #23 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Tue, 14 Feb 2012 22:53:06 -0500

[Message part 1 (text/plain, inline)]

Attached should be a revised patch that addresses all of your suggestions.

Cheers,
   rocky

On Tue, Feb 14, 2012 at 5:49 AM, Jim Meyering <jim <at> meyering.net> wrote:

> Rocky Bernstein wrote:
>
> > On Mon, Feb 13, 2012 at 4:19 AM, Jim Meyering <jim <at> meyering.net> wrote:
> >
> >     Rocky Bernstein wrote:
> >     > It's been a couple of months since I first sent this was sent
> without nary
> >     > an ack. Comments?
> >
> >     Hi Rocky,
> >
> >     su is barely on life support in coreutils.
> >     Meaning that it's no longer really maintained.
> >     We stopped installing it (by default) back in the 2007-2008
> >     time frame (6.9.90, 7.0).  We nearly removed it altogether.
> >
> >     Do you build/install coreutils yourself, or use it via a
> distribution?
> >     If the latter, which distribution?
> >
> > CenOS 6.2 which seems to use coreutils 8.4
> >
> >     I don't remember looking carefully at your patch before,
> >     but glanced through it just now.  Here are some suggestions
> >     if you'd like to pursue it:
> >
> >      - use "error (0, 0, ...", not fprintf (
> >      - indent with spaces, not TABs
> >      - use gnu indentation/formatting style (esp. wrt braces)
> >      - mention the change in NEWS
> >      - for a behavior change like this, we would like a test case
> >
> > The above seems all reasonable and easily doable. But are you suggesting
> is even
> > after getting this into the next coreutils, no OS is likely to use it?
> >
> > The basic idea is that a colleague and I were confused by the fact that
> -l and
> > -p conflict although this was not apparent in the documentation. I wound
> up
> > downloading the source code to understand fully.
> >
> > So something even as simple as mentioning this in doc/coreutils.texi
> would have
> > been helpful (which is the first part of that patch).
> >
> > Going further I realized that it is probably erroneous to supply both -l
> -p and
> > that can be easily warned.
> >
> > But if there is a better place such to report such problems or get code
> > improved, let me know! Thanks.
>
> I see that Fedora still uses su from coreutils, too,
> so this is a worthwhile change.
>

[Message part 2 (text/html, inline)]

[su-l-vs-p.diff (text/x-diff, attachment)]

Message #26 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Philipp Thomas <pth <at> suse.de>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not
	be used together
Date: Tue, 21 Feb 2012 16:03:11 +0100

* Jim Meyering (jim <at> meyering.net) [20120214 11:49]:

> I see that Fedora still uses su from coreutils, too,
> so this is a worthwhile change.

openSUSE and SLES are also using su from coreutils.

Philipp

Message #29 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: bug-coreutils <at> gnu.org, Jim Meyering <jim <at> meyering.net>
Cc: 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Sat, 31 Mar 2012 18:58:52 -0400

[Message part 1 (text/plain, inline)]

Any progress or thoughts on the revised patch?

On Tue, Feb 14, 2012 at 10:53 PM, Rocky Bernstein <rocky <at> gnu.org> wrote:

> Attached should be a revised patch that addresses all of your suggestions.
>
> Cheers,
>    rocky
>
>
> On Tue, Feb 14, 2012 at 5:49 AM, Jim Meyering <jim <at> meyering.net> wrote:
>
>> Rocky Bernstein wrote:
>>
>> > On Mon, Feb 13, 2012 at 4:19 AM, Jim Meyering <jim <at> meyering.net> wrote:
>> >
>> >     Rocky Bernstein wrote:
>> >     > It's been a couple of months since I first sent this was sent
>> without nary
>> >     > an ack. Comments?
>> >
>> >     Hi Rocky,
>> >
>> >     su is barely on life support in coreutils.
>> >     Meaning that it's no longer really maintained.
>> >     We stopped installing it (by default) back in the 2007-2008
>> >     time frame (6.9.90, 7.0).  We nearly removed it altogether.
>> >
>> >     Do you build/install coreutils yourself, or use it via a
>> distribution?
>> >     If the latter, which distribution?
>> >
>> > CenOS 6.2 which seems to use coreutils 8.4
>> >
>> >     I don't remember looking carefully at your patch before,
>> >     but glanced through it just now.  Here are some suggestions
>> >     if you'd like to pursue it:
>> >
>> >      - use "error (0, 0, ...", not fprintf (
>> >      - indent with spaces, not TABs
>> >      - use gnu indentation/formatting style (esp. wrt braces)
>> >      - mention the change in NEWS
>> >      - for a behavior change like this, we would like a test case
>> >
>> > The above seems all reasonable and easily doable. But are you
>> suggesting is even
>> > after getting this into the next coreutils, no OS is likely to use it?
>> >
>> > The basic idea is that a colleague and I were confused by the fact that
>> -l and
>> > -p conflict although this was not apparent in the documentation. I
>> wound up
>> > downloading the source code to understand fully.
>> >
>> > So something even as simple as mentioning this in doc/coreutils.texi
>> would have
>> > been helpful (which is the first part of that patch).
>> >
>> > Going further I realized that it is probably erroneous to supply both
>> -l -p and
>> > that can be easily warned.
>> >
>> > But if there is a better place such to report such problems or get code
>> > improved, let me know! Thanks.
>>
>> I see that Fedora still uses su from coreutils, too,
>> so this is a worthwhile change.
>>
>
>

[Message part 2 (text/html, inline)]

Message #35 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rocky Bernstein <rocky <at> gnu.org>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Sat, 07 Apr 2012 08:15:04 +0200

Rocky Bernstein wrote:
> Any progress or thoughts on the revised patch?

Hi Rocky,
Sorry about the delay.
I haven't forgotten.

I noticed that your test requires typing a root password.
We prefer to avoid that, so that the tests can be run
non-interactively.  That's why some tests are root-only.
(see README for details on running them)
They're marked with require_root_.
At least one is a bourne shell husk that actually does
its work in Perl, so may be a good model.

Message #41 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Thu, 12 Apr 2012 22:43:44 -0400

[Message part 1 (text/plain, inline)]

On Sat, Apr 7, 2012 at 2:15 AM, Jim Meyering <jim <at> meyering.net> wrote:

> Rocky Bernstein wrote:
> > Any progress or thoughts on the revised patch?
>
> Hi Rocky,
> Sorry about the delay.
> I haven't forgotten.
>
> I noticed that your test requires typing a root password.
>

Um, no. I don't know how you got that impression, but did you try running
the test as a non-root or non-privileged user?

Here is what I got when I just tried:

$ (cd tests && make check TESTS=su/p-and-l)
make  check-TESTS
make[1]: Entering directory `/src/external-vcs/coreutils/tests'
make[2]: Entering directory `/src/external-vcs/coreutils/tests'
PASS: su/p-and-l
=============
1 test passed
=============

(Sorry for the delay - I wrote this long enough ago that  I didn't remember
what I did and only now have I had the time to review.)





We prefer to avoid that, so that the tests can be run
> non-interactively.  That's why some tests are root-only.
> (see README for details on running them)
> They're marked with require_root_.
> At least one is a bourne shell husk that actually does
> its work in Perl, so may be a good model.
>

[Message part 2 (text/html, inline)]

Message #47 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Thu, 3 May 2012 20:50:15 -0400

[Message part 1 (text/plain, inline)]

Any word on this?

On Thu, Apr 12, 2012 at 10:43 PM, Rocky Bernstein <rocky <at> gnu.org> wrote:

> On Sat, Apr 7, 2012 at 2:15 AM, Jim Meyering <jim <at> meyering.net> wrote:
>
>> Rocky Bernstein wrote:
>> > Any progress or thoughts on the revised patch?
>>
>> Hi Rocky,
>> Sorry about the delay.
>> I haven't forgotten.
>>
>> I noticed that your test requires typing a root password.
>>
>
> Um, no. I don't know how you got that impression, but did you try running
> the test as a non-root or non-privileged user?
>
> Here is what I got when I just tried:
>
> $ (cd tests && make check TESTS=su/p-and-l)
> make  check-TESTS
> make[1]: Entering directory `/src/external-vcs/coreutils/tests'
> make[2]: Entering directory `/src/external-vcs/coreutils/tests'
> PASS: su/p-and-l
> =============
> 1 test passed
> =============
>
> (Sorry for the delay - I wrote this long enough ago that  I didn't
> remember what I did and only now have I had the time to review.)
>
>
>
>
>
> We prefer to avoid that, so that the tests can be run
>> non-interactively.  That's why some tests are root-only.
>> (see README for details on running them)
>> They're marked with require_root_.
>> At least one is a bourne shell husk that actually does
>> its work in Perl, so may be a good model.
>>
>
>

[Message part 2 (text/html, inline)]

Message #53 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rocky Bernstein <rocky <at> gnu.org>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Fri, 04 May 2012 10:49:47 +0200

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 <rocky <at> gnu.org>
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 --shell=@var{shell}
 @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

Message #59 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: "Voelker, Bernhard" <bernhard.voelker <at> siemens-enterprise.com>
To: Jim Meyering <jim <at> meyering.net>, Rocky Bernstein <rocky <at> gnu.org>
Cc: "10317 <at> debbugs.gnu.org" <10317 <at> debbugs.gnu.org>
Subject: RE: bug#10317: PING - bug#10317: patch to su: -l and -p should not
	be	used together
Date: Fri, 4 May 2012 11:19:56 +0200

Jim Meyering wrote:

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

It (su built from latest git) works here on OpenSuSE-12.1,
so it may be a F17 issue.

OpenSuSE uses su from coreutils BTW.

Have a nice day,
Berny

Message #62 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Fri, 4 May 2012 07:54:50 -0400

[Message part 1 (text/plain, inline)]

On Fri, May 4, 2012 at 4:49 AM, Jim Meyering <jim <at> meyering.net> 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 <rocky <at> gnu.org>
> 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 --shell=@var{shell}
>  @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
>

[Message part 2 (text/html, inline)]

Message #68 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rocky Bernstein <rocky <at> gnu.org>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Fri, 04 May 2012 14:19:25 +0200

Rocky Bernstein wrote:
> On Fri, May 4, 2012 at 4:49 AM, Jim Meyering <jim <at> meyering.net> 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?

That is what I expected, and would certainly require less justification.
If you have a good case for making the semantic change, I'm open to
that, too.

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

Please don't interpret my review delays or my misreading your
patch as anything deliberately unfriendly.  I've been trying to keep
this list as open/accommodating as possible for nearly two decades.
I agree that the long delay is off-putting and apologized for that, twice.

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

If you had read the contribution guidelines in HACKING, you would have
known to run "make syntax-check", which would highlighted three of the
five issues listed above.

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

Message #74 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rocky Bernstein <rocky <at> gnu.org>
Cc: 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Fri, 04 May 2012 14:22:52 +0200

Jim Meyering wrote:
...
> 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.

I've just realized my (silly!) error.
For su to do its job, it must be set-UID root,
and I was running src/su, a non-set-UID file, in place.

Message #77 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Fri, 4 May 2012 10:00:55 -0400

[Message part 1 (text/plain, inline)]

On Fri, May 4, 2012 at 8:19 AM, Jim Meyering <jim <at> meyering.net> wrote:

> Rocky Bernstein wrote:
> > On Fri, May 4, 2012 at 4:49 AM, Jim Meyering <jim <at> meyering.net> 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?
>
> That is what I expected, and would certainly require less justification.
> If you have a good case for making the semantic change, I'm open to
> that, too.
>
> > 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.
>
> Please don't interpret my review delays or my misreading your
> patch as anything deliberately unfriendly.


I didn't say *deliberately unfriendly*. I said there was an *unfriendly
environment*. No doubt, it is unintentional. But for me, for this, it is
there. More below.

 I've been trying to keep
> this list as open/accommodating as possible for nearly two decades.
> I agree that the long delay is off-putting and apologized for that, twice.
>

Understood and accepted. But really there is not much to show that this has
moved forward in the 5 months. Well, okay, for 5 months of time, you have
now at least tried the patch.

So perhaps for the delays, you or someone reading could make the stylistic
changes that would take, what, maybe a few minutes? I promise in the future
with respect to coreutils I will follow coreutils conventions more
carefully. But is this about me getting following coreutils conventions or
is this about getting a "su" problem solved? Right now, if feels like the
former, hence "unfriendly environment".

Also if you had taken the few minutes to make the stylistic changes, you
would have known that things would be properly and it would show that this
you have some *interest* in seeing things improved. Deeds sometimes speak
volumes more than apologies.


>
> > 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.
>
> If you had read the contribution guidelines in HACKING, you would have
> known to run "make syntax-check", which would highlighted three of the
> five issues listed above.
>

Well, that too could have been mentioned 3 months ago -- the *first* time
you talked about stylistic things. Did I say information in drips and
drabs? And it wouldn't have caught the other two. I guess we can both play
the "you did something wrong, let's delay this" game.


> > It's now about 5 months since I snt 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.
>

[Message part 2 (text/html, inline)]

Message #83 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Rocky Bernstein <rocky <at> gnu.org>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Fri, 04 May 2012 16:07:30 +0200

Rocky Bernstein wrote:
> On Fri, May 4, 2012 at 8:19 AM, Jim Meyering <jim <at> meyering.net> wrote:
...
>     Please don't interpret my review delays or my misreading your
>     patch as anything deliberately unfriendly.
>
> I didn't say deliberately unfriendly. I said there was an unfriendly environment
> . No doubt, it is unintentional. But for me, for this, it is there. More below.
>
>      I've been trying to keep
>     this list as open/accommodating as possible for nearly two decades.
>     I agree that the long delay is off-putting and apologized for that, twice.
>
> Understood and accepted. But really there is not much to show that this has
> moved forward in the 5 months. Well, okay, for 5 months of time, you have now at
> least tried the patch.
>
> So perhaps for the delays, you or someone reading could make the stylistic
> changes that would take, what, maybe a few minutes? I promise in the future with
> respect to coreutils I will follow coreutils conventions more carefully. But is
> this about me getting following coreutils conventions or is this about getting a
> "su" problem solved? Right now, if feels like the former, hence "unfriendly
> environment".
>
> Also if you had taken the few minutes to make the stylistic changes, you would
> have known that things would be properly and it would show that this you have
> some interest in seeing things improved. Deeds sometimes speak volumes more than
> apologies.

Um... You seem to have misread.
I did indeed make all of the stylistic adjustments I listed.
I even added a commit log for you and included the output of
"git format-patch" (the "change-set" I mentioned) including
your patch *with those stylistic changes*, so that you could
use that as a basis for any additional changes.

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

Message #89 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: "Voelker, Bernhard" <bernhard.voelker <at> siemens-enterprise.com>
To: Rocky Bernstein <rocky <at> gnu.org>, Jim Meyering <jim <at> meyering.net>
Cc: "10317 <at> debbugs.gnu.org" <10317 <at> debbugs.gnu.org>
Subject: RE: bug#10317: PING - bug#10317: patch to su: -l and -p should not
	be	used together
Date: Fri, 4 May 2012 16:12:11 +0200

Rocky Bernstein wrote:

> So perhaps for the delays, you or someone reading could make the stylistic
> changes that would take, what, maybe a few minutes?

IMHO no.
As Jim is the CU maintainer, he (or Padraig or someone else) will push
changes to git. As it's always good to have someone else to review a change
- even if it's (seems to be) a trivial one- , such a change should be
reviewed again ... introducing another round.
Therefore it's better when you make the change.

Have a nice day,
Berny

Message #92 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rocky Bernstein <rocky <at> gnu.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-coreutils <at> gnu.org, 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: PING - bug#10317: patch to su: -l and -p should not be
	used together
Date: Fri, 4 May 2012 10:20:35 -0400

[Message part 1 (text/plain, inline)]

On Fri, May 4, 2012 at 10:07 AM, Jim Meyering <jim <at> meyering.net> wrote:

> Rocky Bernstein wrote:
> > On Fri, May 4, 2012 at 8:19 AM, Jim Meyering <jim <at> meyering.net> wrote:
> ...
> >     Please don't interpret my review delays or my misreading your
> >     patch as anything deliberately unfriendly.
> >
> > I didn't say deliberately unfriendly. I said there was an unfriendly
> environment
> > . No doubt, it is unintentional. But for me, for this, it is there. More
> below.
> >
> >      I've been trying to keep
> >     this list as open/accommodating as possible for nearly two decades.
> >     I agree that the long delay is off-putting and apologized for that,
> twice.
> >
> > Understood and accepted. But really there is not much to show that this
> has
> > moved forward in the 5 months. Well, okay, for 5 months of time, you
> have now at
> > least tried the patch.
> >
> > So perhaps for the delays, you or someone reading could make the
> stylistic
> > changes that would take, what, maybe a few minutes? I promise in the
> future with
> > respect to coreutils I will follow coreutils conventions more carefully.
> But is
> > this about me getting following coreutils conventions or is this about
> getting a
> > "su" problem solved? Right now, if feels like the former, hence
> "unfriendly
> > environment".
> >
> > Also if you had taken the few minutes to make the stylistic changes, you
> would
> > have known that things would be properly and it would show that this you
> have
> > some interest in seeing things improved. Deeds sometimes speak volumes
> more than
> > apologies.
>
> Um... You seem to have misread.
> I did indeed make all of the stylistic adjustments I listed.
> I even added a commit log for you and included the output of
> "git format-patch" (the "change-set" I mentioned) including
> your patch *with those stylistic changes*, so that you could
> use that as a basis for any additional changes.
>

Ok - my bad. Thanks.


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

[Message part 2 (text/html, inline)]

Message #98 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: "util-linux <at> vger.kernel.org" <util-linux <at> vger.kernel.org>
Cc: 10317 <at> debbugs.gnu.org
Subject: su: -l and -p should not be used together
Date: Thu, 23 May 2013 12:57:05 +0200

tag 10317 wontfix
close 10317
stop

A while ago, there has been a bug report to coreutils regarding
the unlucky use of the -p and the -l option (or a bare "-") with su:
  http://bugs.gnu.org/10317

As su is not part of coreutils anymore, I'm hereby closing the bug
there, and forward this issue to util-linux where su is now maintained.

@Karel:
do you think it's worth a warning/error?
However, the agreement was that the behaviour should at least
be documented.

Have a nice day,
Berny

Message #101 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Karel Zak <kzak <at> redhat.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: "util-linux <at> vger.kernel.org" <util-linux <at> vger.kernel.org>,
	10317 <at> debbugs.gnu.org
Subject: Re: su: -l and -p should not be used together
Date: Wed, 29 May 2013 11:40:10 +0200

On Thu, May 23, 2013 at 12:57:05PM +0200, Bernhard Voelker wrote:
> A while ago, there has been a bug report to coreutils regarding
> the unlucky use of the -p and the -l option (or a bare "-") with su:
>   http://bugs.gnu.org/10317
> 
> As su is not part of coreutils anymore, I'm hereby closing the bug
> there, and forward this issue to util-linux where su is now maintained.
> 
> @Karel:
> do you think it's worth a warning/error?
> However, the agreement was that the behaviour should at least
> be documented.

Thanks for the report, I have added note to the man page as well as
warning to the code.

    Karel

From 3e5c0a2db233b726ca80d37aad9eeca8bae144d4 Mon Sep 17 00:00:00 2001
From: Karel Zak <kzak <at> redhat.com>
Date: Wed, 29 May 2013 11:32:58 +0200
Subject: [PATCH] su: ignore --preserve-environment, it's mutually exclusive to
 --login

Addresses: http://bugs.gnu.org/10317
Reported-by: Bernhard Voelker <mail <at> bernhard-voelker.de>
Signed-off-by: Karel Zak <kzak <at> redhat.com>
---
 login-utils/su-common.c | 5 +++++
 login-utils/su.1        | 1 +
 2 files changed, 6 insertions(+)

diff --git a/login-utils/su-common.c b/login-utils/su-common.c
index ba2a616..a41d015 100644
--- a/login-utils/su-common.c
+++ b/login-utils/su-common.c
@@ -810,6 +810,11 @@ su_main (int argc, char **argv, int mode)
       ++optind;
     }
 
+  if (simulate_login && !change_environment) {
+    warnx(_("ignore --preserve-environment, it's mutually exclusive to --login."));
+    change_environment = true;
+  }
+
   switch (su_mode) {
   case RUNUSER_MODE:
     if (runuser_user) {
diff --git a/login-utils/su.1 b/login-utils/su.1
index c82b941..eab1a6f 100644
--- a/login-utils/su.1
+++ b/login-utils/su.1
@@ -98,6 +98,7 @@ Preserves the whole environment, ie does not set
 .B USER
 nor
 .BR LOGNAME .
+The option is ignored if the option \fB\-\-login\fR is specified.
 .TP
 \fB\-s\fR \fISHELL\fR, \fB\-\-shell\fR=\fISHELL\fR
 Runs the specified shell instead of the default.  The shell to run is
-- 
1.8.1.4


-- 
 Karel Zak  <kzak <at> redhat.com>
 http://karelzak.blogspot.com

Message #104 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Karel Zak <kzak <at> redhat.com>
Cc: "util-linux <at> vger.kernel.org" <util-linux <at> vger.kernel.org>,
	10317 <at> debbugs.gnu.org
Subject: Re: su: -l and -p should not be used together
Date: Wed, 29 May 2013 11:55:10 +0200

Hi Karel,

On 05/29/2013 11:40 AM, Karel Zak wrote:
> diff --git a/login-utils/su-common.c b/login-utils/su-common.c
> index ba2a616..a41d015 100644
> --- a/login-utils/su-common.c
> +++ b/login-utils/su-common.c
> @@ -810,6 +810,11 @@ su_main (int argc, char **argv, int mode)
>        ++optind;
>      }
>  
> +  if (simulate_login && !change_environment) {
> +    warnx(_("ignore --preserve-environment, it's mutually exclusive to --login."));
> +    change_environment = true;
> +  }
> +

thanks.

As the change was in su-common.c, this also affects runuser.
I think this should then be documented in runuser.1, too, right?

Have a nice day,
Berny

Message #107 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Karel Zak <kzak <at> redhat.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: "util-linux <at> vger.kernel.org" <util-linux <at> vger.kernel.org>,
	10317 <at> debbugs.gnu.org
Subject: Re: su: -l and -p should not be used together
Date: Fri, 7 Jun 2013 11:28:07 +0200

On Wed, May 29, 2013 at 11:55:10AM +0200, Bernhard Voelker wrote:
> As the change was in su-common.c, this also affects runuser.
> I think this should then be documented in runuser.1, too, right?

 Good point, fixed.

    Karel

-- 
 Karel Zak  <kzak <at> redhat.com>
 http://karelzak.blogspot.com

Message #110 received at 10317 <at> debbugs.gnu.org (full text, mbox):

From: Assaf Gordon <assafgordon <at> gmail.com>
Cc: 10317 <at> debbugs.gnu.org
Subject: Re: bug#10317: su: -l and -p should not be used together
Date: Mon, 15 Oct 2018 09:06:41 -0600

close 10317
stop

(triaging old bugs)

su(1) is no longer maintained in coreutils,
and most distributions have switched to util-linux's su(1).

Closing this bug.

regards,
 - assaf

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.