Bug 1448 - Report wrong command-line options
Summary: Report wrong command-line options
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: scp (show other bugs)
Version: 4.6p1
Hardware: Other Linux
: P2 normal
Assignee: Assigned to nobody
URL:
Keywords: low-hanging-fruit
Depends on:
Blocks: V_6_3
  Show dependency treegraph
 
Reported: 2008-03-18 03:25 AEDT by bonniot
Modified: 2015-08-11 23:04 AEST (History)
2 users (show)

See Also:


Attachments
Factor out portability changes in getopt.c (3.73 KB, patch)
2013-05-10 16:09 AEST, Darren Tucker
no flags Details | Diff
import getopt_long.c from openbsd (14.80 KB, patch)
2013-05-10 16:10 AEST, Darren Tucker
no flags Details | Diff
apply portability changes to getopt_long.c switch over to it and remove getopt.c (8.55 KB, patch)
2013-05-10 16:11 AEST, Darren Tucker
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description bonniot 2008-03-18 03:25:22 AEDT
It seems that currently scp just silently ignores command-line options that it does not understand. That is surprising and possibly dangerous or damageable (since once will expect the option to have been taken into account).

It seems pretty standard for a tool to fail on unknown options, but since that might break existing scripts, maybe starting with reporting a warning would be the best compromise.
Comment 1 bonniot 2008-11-04 05:59:56 AEDT
To clarify, this is the case with long options (starting with --...). Example:
touch a; scp --foobarbaz a b
does not report any error or warning.
Comment 2 Damien Miller 2012-03-09 11:33:49 AEDT
scp does report unknown options:

$ scp -X a: b:
scp: illegal option -- X
usage: scp [-12346BCpqrv] [-c cipher] [-F ssh_config] [-i identity_file]
           [-l limit] [-o ssh_option] [-P port] [-S program]
           [[user@]host1:]file1 ... [[user@]host2:]file2

It also reports --options options on OpenBSD:

$ scp --blah a: b:
scp: illegal option -- -
usage: scp [-12346BCpqrv] [-c cipher] [-F ssh_config] [-i identity_file]
           [-l limit] [-o ssh_option] [-P port] [-S program]
           [[user@]host1:]file1 ... [[user@]host2:]file2

I think what is happening that GNU getopt is swallowing the long options before scp sees them. There isn't much scp could do here except perhaps avoid the system getopt() for its own and I don't think this is worth it.
Comment 3 bonniot 2012-03-12 19:58:51 AEDT
I don't know scp interracts with getopt. But it seems to me that either getopt offers a way to know that that long option was given, and scp could fail/warn if it does not know the option. Or getopt does not allow that, and a bug report/feature request is in order.
Comment 4 Darren Tucker 2013-05-10 14:27:32 AEST
whatever it is, it doesn't seem to be gnu-getopt specific.  or if it is, it's not enabled with the default set of defines.

#include <stdio.h>
#include <unistd.h>

int main(int argc, char **argv)
{
	int ch;
	while ((ch = getopt(argc, argv, "abc:")) != -1) {
		printf("opt %c optarg %s\n", ch, optarg ? optarg : "(null)");
	}
}

gives me:
$ ./a.out --foo      
./a.out: invalid option -- '-'
opt ? optarg (null)
./a.out: invalid option -- 'f'
opt ? optarg (null)
./a.out: invalid option -- 'o'
opt ? optarg (null)
./a.out: invalid option -- 'o'
opt ? optarg (null)

whatever it is, I think it's in our compat library:
$ nm scp | grep -i getop
08051710 T BSDgetopt
Comment 5 Darren Tucker 2013-05-10 14:32:33 AEST
sure looks like something of ours:

$ gcc getopt-test.c -Dgetopt=BSDgetopt openbsd-compat/libopenbsd-compat.a
$ ./a.out -a
opt a optarg (null)
$ ./a.out --foo
$
Comment 6 Darren Tucker 2013-05-10 15:20:49 AEST
ok, so it turns out the original upstream source in OpenBSD's getopt.c had the same behaviour, but was replaced in 2004 with getopt_long.c which doesn't.  For long-term maintainability I think we should switch.

What I'm going to do is
1) factor out the portability changes in getopt.c
2) add getopt_long.c to openssh and remove getopt.c
3) pull the portability changes into getopt_long.c
Comment 7 Darren Tucker 2013-05-10 16:09:28 AEST
Created attachment 2257 [details]
Factor out portability changes in getopt.c
Comment 8 Darren Tucker 2013-05-10 16:10:05 AEST
Created attachment 2258 [details]
import getopt_long.c from openbsd
Comment 9 Darren Tucker 2013-05-10 16:11:10 AEST
Created attachment 2259 [details]
apply portability changes to getopt_long.c switch over to it and remove getopt.c
Comment 10 Darren Tucker 2013-05-10 16:29:47 AEST
With these changes I get the following for your testcase:

$ touch a; ./scp --foobarbaz a b
unknown option -- -
usage: scp [-12346BCpqrv] [-c cipher] [-F ssh_config] [-i identity_file]
           [-l limit] [-o ssh_option] [-P port] [-S program]
           [[user@]host1:]file1 ... [[user@]host2:]file2

Thanks for the report (and patience :-).  These changes will be in the 6.3p1 release.
Comment 11 bonniot 2013-05-10 19:30:52 AEST
Thank you for the fix :)

Any chance the portability fixes could be applied upstream?
Comment 12 Darren Tucker 2013-05-10 20:07:11 AEST
It's already as upstream as it gets.  The file with the bug in openbsd is dead, and the current version of the current file is fine.
Comment 13 Damien Miller 2015-08-11 23:04:46 AEST
Set all RESOLVED bugs to CLOSED with release of OpenSSH 7.1