From 989f293a5d568304c701c89055baf6ab38909c47 Mon Sep 17 00:00:00 2001
From: Manlio Perillo <manlio.perillo@gmail.com>
Date: Fri, 15 Mar 2024 09:26:38 +0100
Subject: [PATCH] configuration: improve handling of positional arguments

Currently getopt_long is configured to support mixing options and
positional arguments, but kcov don't use the special "--" argument to
tell getopt_long when to stop parsing the executable arguments.
Instead the implementation tries to scan the arguments to find an
executable, but the implementation is flawed because there are two
required positional arguments, and both may be executable.

In order to handle positional arguments correctly, configure getopt_long
to stop parsing after encountering a positional argument.

This change greatly simplified the code, since the pre scanning can be
removed.  However it may break the workflow of some users.

Add a regression test for issue #414

Issue #414: kcov: error when out-dir is an executable
---
 src/configuration.cc | 55 ++++++++++----------------------------------
 tests/tools/basic.py |  8 +++++++
 2 files changed, 20 insertions(+), 43 deletions(-)

diff --git a/src/configuration.cc b/src/configuration.cc
index 8d831a09..a778d72d 100644
--- a/src/configuration.cc
+++ b/src/configuration.cc
@@ -19,14 +19,6 @@ using namespace kcov;
 
 extern "C" const char *kcov_version;
 
-// This function can't be defined in utils.cc due to the use of the singleton
-// pattern. It will cause a linker error when buildind tests, since the utils.cc
-// file is also included in the test source files.
-static bool is_supported_executable(const std::string path)
-{
-	return kcov::IParserManager::getInstance().matchParser(path) != 0;
-}
-
 class Configuration : public IConfiguration
 {
 public:
@@ -170,41 +162,12 @@ class Configuration : public IConfiguration
 		{ 0, 0, 0, 0 } };
 		unsigned int afterOpts = 0;
 		unsigned int extraNeeded = 2;
-		unsigned int lastArg;
+		bool printUsage = false;
 
 		setupDefaults();
 
 		setKey("kcov-binary-path", get_real_path(argv[0]));
 
-		/* Scan through the parameters for a supported executable file: That
-		 * will be the second last argument in the list.
-		 *
-		 * After that it's arguments to the external program.
-		 */
-		for (lastArg = 1; lastArg < argc; lastArg++)
-		{
-			std::string arg = std::string(argv[lastArg]);
-			int ret;
-			std::string path;
-
-			// Skip options.
-			if ((arg.size() > 0) && (arg[0] == '-'))
-				continue;
-
-			ret = look_path(arg, &path);
-			if (ret < 0)
-				continue;
-
-			if (is_supported_executable(path))
-			{
-				// Intentional memory leak
-				argv[lastArg] = xstrdup(path.c_str());
-				break;
-			}
-		}
-
-		bool printUsage = false;
-
 		/* Hooray for reentrancy... */
 		optind = 0;
 		optarg = 0;
@@ -213,7 +176,11 @@ class Configuration : public IConfiguration
 			int option_index = 0;
 			int c;
 
-			c = getopt_long(lastArg, (char **) argv, "hp:s:l:t:", long_options,
+			// Force getopt_long to stop as soon as a nonoption argument is
+			// encountered.  This will ensure correct parsing of positional
+			// arguments without having to use "--" to stop parsing the
+			// executable arguments.
+			c = getopt_long(argc, (char **) argv, "+hp:s:l:t:", long_options,
 					&option_index);
 
 			/* No more options */
@@ -492,7 +459,6 @@ class Configuration : public IConfiguration
 		}
 		else
 		{
-			std::string binaryName;
 			std::string path;
 
 			if (keyAsInt("attach-pid") == 0)
@@ -513,8 +479,11 @@ class Configuration : public IConfiguration
 				}
 			}
 
-			path = get_real_path(path);
-			std::pair<std::string, std::string> tmp = split_path(path);
+			std::string binaryName;
+			std::string binaryPath = path;
+
+			(void)look_path(path, &binaryPath);
+			std::pair<std::string, std::string> tmp = split_path(binaryPath);
 
 			setKey("binary-path", tmp.first);
 			binaryName = tmp.second;
@@ -523,7 +492,7 @@ class Configuration : public IConfiguration
 			{
 				setKey("target-directory",
 						fmt("%s/%s.%08zx", outDirectory.c_str(), binaryName.c_str(),
-								(size_t)hash_file(path)));
+								(size_t)hash_file(get_real_path(binaryPath))));
 			}
 			else
 			{
diff --git a/tests/tools/basic.py b/tests/tools/basic.py
index d7ccda19..50772209 100644
--- a/tests/tools/basic.py
+++ b/tests/tools/basic.py
@@ -26,3 +26,11 @@ def runTest(self):
         dom = parse_cobertura.parseFile(testbase.outbase + "/kcov/main/cobertura.xml")
         assert parse_cobertura.hitsPerLine(dom, "second.py", 34) == 2
         assert noKcovRv, rv
+
+# Issue #414
+class OutDirectoryIsExecutable(testbase.KcovTestCase):
+    def runTest(self):
+        self.setUp()
+        rv,o = self.do(testbase.kcov + " echo python -V")
+
+        assert rv == 0