test a = a b: undetected error

[McDutchie]

In https://bugs.debian.org/1068340, Vincent Lefevre reports:

Package: ksh93u+m
Version: 1.0.8-1
Severity: normal

$ test a = a b ; echo $?
0

while this should be an error.

Further discussion (thread): https://www.mail-archive.com/[email protected]/msg12484.html

[McDutchie]

The behaviour is identical in the original Bourne shell (tested /bin/sh on Solaris 9) and on ksh88.

[vinc17fr]

However, it could have been just a missing test for error (i.e. possibly a bug unknown at that time). Or was this done on purpose and documented? Or at least a case that was checked in the testsuite? And are there scripts that expect this behavior? I could imagine that after the equal, one may have something with a * character to perform an expansion (globbing), so that the test would be done against the first expanded word.

[McDutchie]

It's not documented and not checked for in the test suite. I have no idea if it was done on purpose or not. As long as parentheses or -a/-o are not used, it seems to accept an arbitrary number of superfluous arguments that it will ignore (like test a = a b c d e f g), and this applies to unary operators as well (like test -e /dev/null foo bar baz). So maybe your globbing hypothesis is correct.

[McDutchie]

Here is a patch that should fix the bug. It's a kludge, but it avoids messing with existing code to minimise the chance of unwanted side effects. All the regression tests pass. Please test.

I've commented out the check for POSIX mode so I can easily run the fix against the regression tests. I'm also undecided whether this should be fixed unconditionally or in POSIX mode only.

diff --git a/src/cmd/ksh93/bltins/test.c b/src/cmd/ksh93/bltins/test.c
index 48b391d4e..cc81ece24 100644
--- a/src/cmd/ksh93/bltins/test.c
+++ b/src/cmd/ksh93/bltins/test.c
@@ -104,6 +104,28 @@ static int test_strmatch(const char *str, const char *pat)
 	return n;
 }
 
+static void check_toomanyops(char *argv[])
+{
+	if(c_eq(argv[0],'(') || !argv[1] || !argv[2] || !argv[3])
+		return;
+	/* superfluous args after simple binary expression */
+	if(sh_lookup(argv[2],shtab_testops))
+	{
+		if(argv[4] && !(sh_lookup(argv[4],shtab_testops) & TEST_ANDOR))
+		{
+			errormsg(SH_DICT,ERROR_exit(2),e_toomanyops);
+			UNREACHABLE();
+		}
+		return;
+	}
+	/* superfluous args after simple unary expression */
+	if(argv[1][0]=='-' && isalpha(argv[1][1]) && !argv[1][2] && !(sh_lookup(argv[3],shtab_testops) & TEST_ANDOR))
+	{
+		errormsg(SH_DICT,ERROR_exit(2),e_toomanyops);
+		UNREACHABLE();
+	}
+}
+
 int b_test(int argc, char *argv[],Shbltin_t *context)
 {
 	struct test tdata;
@@ -121,6 +143,7 @@ int b_test(int argc, char *argv[],Shbltin_t *context)
 			errormsg(SH_DICT,ERROR_exit(2),e_missing,"']'");
 			UNREACHABLE();
 		}
+		argv[argc] = NULL;
 	}
 	if(argc <= 1)
 	{
@@ -140,6 +163,8 @@ int b_test(int argc, char *argv[],Shbltin_t *context)
 		}
 	}
 	not = c_eq(cp,'!');
+//	if(sh_isoption(SH_POSIX))
+		check_toomanyops(argv + not);
 	/* POSIX portion for test */
 	switch(argc)
 	{

[vinc17fr]

For test t -a b = b, I get test: too many operands instead of just an exit status 0 as with other shells.

[McDutchie]

Thanks for testing the patch. Here is version two that should fix that. It also fixes test -n -a -z.

There are very likely still problems left, though. Fixing this mess without unwanted side effects is fiendishly difficult, if not impossible.

diff --git a/src/cmd/ksh93/bltins/test.c b/src/cmd/ksh93/bltins/test.c
index 48b391d4e..32a1c1141 100644
--- a/src/cmd/ksh93/bltins/test.c
+++ b/src/cmd/ksh93/bltins/test.c
@@ -104,6 +104,29 @@ static int test_strmatch(const char *str, const char *pat)
 	return n;
 }
 
+static void check_toomanyops(char *argv[])
+{
+	unsigned n;
+	if(c_eq(argv[0],'(') || !argv[1] || !argv[2] || !argv[3])
+		return;
+	/* superfluous args after simple binary expression */
+	if((n = sh_lookup(argv[2],shtab_testops)) && !(n & TEST_ANDOR))
+	{
+		if(argv[4] && !(sh_lookup(argv[4],shtab_testops) & TEST_ANDOR))
+		{
+			errormsg(SH_DICT,ERROR_exit(2),e_toomanyops);
+			UNREACHABLE();
+		}
+		return;
+	}
+	/* superfluous args after simple unary expression */
+	if(argv[1][0]=='-' && isalpha(argv[1][1]) && !argv[1][2] && !(n & TEST_ANDOR) && !(sh_lookup(argv[3],shtab_testops) & TEST_ANDOR))
+	{
+		errormsg(SH_DICT,ERROR_exit(2),e_toomanyops);
+		UNREACHABLE();
+	}
+}
+
 int b_test(int argc, char *argv[],Shbltin_t *context)
 {
 	struct test tdata;
@@ -121,6 +144,7 @@ int b_test(int argc, char *argv[],Shbltin_t *context)
 			errormsg(SH_DICT,ERROR_exit(2),e_missing,"']'");
 			UNREACHABLE();
 		}
+		argv[argc] = NULL;
 	}
 	if(argc <= 1)
 	{
@@ -140,6 +164,8 @@ int b_test(int argc, char *argv[],Shbltin_t *context)
 		}
 	}
 	not = c_eq(cp,'!');
+//	if(sh_isoption(SH_POSIX))
+		check_toomanyops(argv + not);
 	/* POSIX portion for test */
 	switch(argc)
 	{

[vinc17fr]

This new patch looks fine. It is nice to see that it detects the following error:

$ test b = b c -a "" ; echo $?
/home/vlefevre/ksh/arch/linux.i386-64/dyn/bin/ksh: test: too many operands
2

In the current Debian/unstable version, the result is inconsistent. First,

$ test b = b -a "" ; echo $?
1

meaning that the -a as the AND operator overrides the interpretation of additional, ignored arguments. But this is no longer the case here:

$ test b = b c -a "" ; echo $?
0

Strangely, the error is detected here:

$ test b = b -a b = b c -a "" ; echo $?
ksh93: test: incorrect syntax
2

With the patch, this becomes consistent, with an error being always reported in case of too many arguments for the = test (not always the same error message: "too many operands" or "incorrect syntax"; but both error messages are correct).