Merge lp:~marcustomlinson/unity-scopes-api/fix_relative_custom_exec_args into lp:unity-scopes-api/devel

[Marcus Tomlinson]

Loop through each argument of the custom scope runner command and ensure that all path arguments are absolute

[Paweł Stołowski (stolowski)]

Do you mind adding some tests for convert_exec_rel_to_abs (and split_args - would be good to have a check for arguments which contain escaped spaces, e.g. Foo\ Bar.) in the Utils test (test/gtest/scopes/internal/Utils/Utils_test.cpp)? That should allow for a much better test than with a test scope.
Other than that looks good and clean!

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:630
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1548/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-amd64-ci/89
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-armhf-ci/87
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-armhf-ci/87/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-i386-ci/89

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1548/rebuild

review: Approve (continuous-integration)

[lp:~marcustomlinson/unity-scopes-api/fix_relative_custom_exec_args]

631. By Marcus Tomlinson on 2015-09-15

Added tests

[Marcus Tomlinson (marcustomlinson)]

> Do you mind adding some tests for convert_exec_rel_to_abs (and split_args -
> would be good to have a check for arguments which contain escaped spaces, e.g.
> Foo\ Bar.) in the Utils test
> (test/gtest/scopes/internal/Utils/Utils_test.cpp)? That should allow for a
> much better test than with a test scope.
> Other than that looks good and clean!

Added tests.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:631
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1550/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-amd64-ci/91
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-armhf-ci/89
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-armhf-ci/89/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-i386-ci/91

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1550/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

Awesome, thanks! +1

review: Approve

=== modified file 'include/unity/scopes/internal/Utils.h'
--- include/unity/scopes/internal/Utils.h	2015-04-10 03:55:25 +0000
+++ include/unity/scopes/internal/Utils.h	2015-09-15 07:36:31 +0000
@@ -22,6 +22,8 @@
 #include <string>
 #include <sstream>
 
+#include <boost/filesystem.hpp>
+
 namespace unity
 {
 
@@ -57,6 +59,9 @@
 
 void make_directories(std::string const& path_name, mode_t mode);
 
+std::vector<std::string> split_exec_args(std::string const& id, std::string const& custom_exec);
+std::string convert_exec_rel_to_abs(std::string const& id, boost::filesystem::path const& scope_dir, std::string const& custom_exec);
+
 } // namespace internal
 
 } // namespace scopes

=== modified file 'scoperegistry/scoperegistry.cpp'
--- scoperegistry/scoperegistry.cpp	2015-07-01 11:02:46 +0000
+++ scoperegistry/scoperegistry.cpp	2015-09-15 07:36:31 +0000
@@ -259,30 +259,6 @@
     }
 }
 
-string get_first_component(string const& id, string const& custom_exec)
-{
-    if (custom_exec.empty())
-    {
-        throw unity::InvalidArgumentException("Invalid scope runner executable for scope: " + id);
-    }
-
-    wordexp_t exp;
-    string result;
-
-    // Split command into program and args
-    if (wordexp(custom_exec.c_str(), &exp, WRDE_NOCMD) == 0)
-    {
-        util::ResourcePtr<wordexp_t*, decltype(&wordfree)> free_guard(&exp, wordfree);
-        result = exp.we_wordv[0];
-    }
-    else
-    {
-        throw unity::InvalidArgumentException("Invalid scope runner executable for scope: " + id);
-    }
-
-    return result;
-}
-
 // For each scope, open the config file for the scope, create the metadata info from the config,
 // and add an entry to the RegistryObject.
 // If the scope uses settings, also parse the settings file and add the settings to the metadata.
@@ -435,33 +411,7 @@
 
     try
     {
-        auto custom_exec = sc.scope_runner();
-        filesystem::path program(get_first_component( scope.first, custom_exec));
-        if (program.is_relative())
-        {
-            // First look inside the arch-specific directory
-            if (filesystem::exists(scope_dir / DEB_HOST_MULTIARCH / program))
-            {
-                // Join the full command, not just the program path
-                exec_data.custom_exec = (scope_dir / DEB_HOST_MULTIARCH / custom_exec).native();
-            }
-            // Next try in the non arch-aware directory
-            else if (filesystem::exists(scope_dir / program))
-            {
-                // Join the full command, not just the program path
-                exec_data.custom_exec = (scope_dir / custom_exec).native();
-            }
-            else
-            {
-                throw unity::InvalidArgumentException(
-                        "Nonexistent scope runner '" + custom_exec
-                                + "' executable for scope: " + scope.first);
-            }
-        }
-        else
-        {
-            exec_data.custom_exec = custom_exec;
-        }
+        exec_data.custom_exec = convert_exec_rel_to_abs(scope.first, scope_dir, sc.scope_runner());
     }
     catch (NotFoundException const&)
     {

=== modified file 'src/scopes/internal/Utils.cpp'
--- src/scopes/internal/Utils.cpp	2015-04-10 03:55:25 +0000
+++ src/scopes/internal/Utils.cpp	2015-09-15 07:36:31 +0000
@@ -19,12 +19,14 @@
 #include <unity/scopes/internal/Utils.h>
 #include <unity/scopes/ScopeExceptions.h>
 #include <unity/UnityExceptions.h>
+#include <unity/util/ResourcePtr.h>
 
 #include <boost/filesystem.hpp>
 
 #include <iomanip>
 #include <locale>
 #include <mutex>
+#include <wordexp.h>
 
 #include <sys/stat.h>
 
@@ -190,6 +192,87 @@
     }
 }
 
+vector<string> split_exec_args(string const& id, string const& custom_exec)
+{
+    if (custom_exec.empty())
+    {
+        throw unity::InvalidArgumentException("Invalid empty executable for scope: '" + id + "'");
+    }
+
+    wordexp_t exp;
+    vector<string> result;
+
+    // Split command into program and args
+    if (wordexp(custom_exec.c_str(), &exp, WRDE_NOCMD) == 0)
+    {
+        util::ResourcePtr<wordexp_t*, decltype(&wordfree)> free_guard(&exp, wordfree);
+        for (size_t i = 0; i < exp.we_wordc; ++i )
+        {
+            auto argument = string(exp.we_wordv[i]);
+            if(argument.find_first_of(' ') != std::string::npos)
+            {
+                // This argument contains spaces, enclose it in quotation marks
+                result.emplace_back("\"" + argument + "\"");
+            }
+            else
+            {
+                result.emplace_back(argument);
+            }
+        }
+    }
+    else
+    {
+        throw unity::InvalidArgumentException("Invalid executable for scope: '" + id + "'");
+    }
+
+    return result;
+}
+
+string convert_exec_rel_to_abs(string const& id, boost::filesystem::path const& scope_dir, string const& custom_exec)
+{
+    string result;
+
+    // Loop through each argument of the scope runner command and ensure all path args are absolute
+    auto custom_exec_args = split_exec_args(id, custom_exec);
+    for (auto custom_exec_arg : custom_exec_args)
+    {
+        boost::filesystem::path argument(custom_exec_arg);
+        if (argument.is_relative())
+        {
+            // First look inside the arch-specific directory
+            if (boost::filesystem::exists(scope_dir / DEB_HOST_MULTIARCH / argument))
+            {
+                // Append the argument as a relative path
+                result += (scope_dir / DEB_HOST_MULTIARCH / argument).native() + " ";
+            }
+            // Next try in the non arch-aware directory
+            else if (boost::filesystem::exists(scope_dir / argument))
+            {
+                // Append the argument as a relative path
+                result += (scope_dir / argument).native() + " ";
+            }
+            // If this is the first argument (program name) it must exist, throw here
+            else if (result.empty())
+            {
+                throw unity::InvalidArgumentException(
+                        "Nonexistent scope runner executable: '" + custom_exec_arg
+                                + "' for scope: '" + id + "'");
+            }
+            // Otherwise just append the argument as is
+            else
+            {
+                result += custom_exec_arg + " ";
+            }
+        }
+        else
+        {
+            result += custom_exec_arg + " ";
+        }
+    }
+    result.resize(result.size() - 1);
+    return result;
+}
+
 } // namespace internal
 
 } // namespace scopes

=== modified file 'test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp'
--- test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp	2014-12-05 03:05:37 +0000
+++ test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp	2015-09-15 07:36:31 +0000
@@ -25,6 +25,9 @@
 #include <unity/scopes/Runtime.h>
 #include <unity/scopes/PreviewReply.h>
 
+#include <boost/filesystem.hpp>
+
+using namespace boost;
 using namespace std;
 using namespace unity::scopes;
 
@@ -102,10 +105,11 @@
     std::string scope_config = argc > 2 ? argv[2] : "FAIL";
     std::string rt_config = argc > 3 ? argv[3] : "FAIL";
     std::string unused_arg_2 = argc > 4 ? argv[4] : "FAIL";
+    filesystem::path abs_path_arg(argc > 5 ? argv[5] : "FAIL");
 
     // In order to test that custom exec splitting works, we check here that our arbitrary arguments
     // have been delivered to us as expected.
-    if (unused_arg_1 == "unused arg 1" && unused_arg_2 == "unused arg 2")
+    if (unused_arg_1 == "unused arg 1" && unused_arg_2 == "unused arg 2" && filesystem::exists(abs_path_arg))
     {
         MyScope scope;
         auto runtime = Runtime::create_scope_runtime("testscopeB", rt_config);

=== modified file 'test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in'
--- test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in	2014-12-08 15:17:04 +0000
+++ test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in	2015-09-15 07:36:31 +0000
@@ -6,5 +6,5 @@
 Icon = data/scope-B.Icon
 SearchHint = scope-B.SearchHint
 HotKey = scope-B.HotKey
-ScopeRunner = testscopeB "unused arg 1" %S %R unused' 'arg' '2
+ScopeRunner = testscopeB "unused arg 1" %S %R unused' 'arg' '2 ./testscopeB.ini
 IdleTimeout = 2

=== modified file 'test/gtest/scopes/internal/Utils/Utils_test.cpp'
--- test/gtest/scopes/internal/Utils/Utils_test.cpp	2015-04-10 03:55:25 +0000
+++ test/gtest/scopes/internal/Utils/Utils_test.cpp	2015-09-15 07:36:31 +0000
@@ -132,3 +132,89 @@
 
     chmod(child.c_str(), 0777);  // Don't leave the dir behind without permissions.
 }
+
+TEST(Utils, split_exec_args)
+{
+    // Test empty executable
+    try
+    {
+        split_exec_args("test", "");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Invalid empty executable for scope: 'test'", e.what());
+    }
+
+    // Test invalid executable
+    try
+    {
+        split_exec_args("test", "\"");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Invalid executable for scope: 'test'", e.what());
+    }
+
+    // Test argument splitting
+    auto exec_args = split_exec_args("test", "/path\\ to/exec' 'file arg \"arg 2\" arg' '3 arg\\ 4");
+    ASSERT_EQ(5, exec_args.size());
+    EXPECT_STREQ("\"/path to/exec file\"", exec_args[0].c_str());
+    EXPECT_STREQ("arg", exec_args[1].c_str());
+    EXPECT_STREQ("\"arg 2\"", exec_args[2].c_str());
+    EXPECT_STREQ("\"arg 3\"", exec_args[3].c_str());
+    EXPECT_STREQ("\"arg 4\"", exec_args[4].c_str());
+}
+
+TEST(Utils, convert_exec_rel_to_abs)
+{
+    // Test empty executable
+    try
+    {
+        convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Invalid empty executable for scope: 'test'", e.what());
+    }
+
+    // Test invalid executable
+    try
+    {
+        convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "\"");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Invalid executable for scope: 'test'", e.what());
+    }
+
+    // Test nonexistent executable
+    try
+    {
+        convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "noexec");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Nonexistent scope runner executable: 'noexec' for scope: 'test'", e.what());
+    }
+
+    // Test absolute executable
+    auto abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "/bin/bash");
+    EXPECT_STREQ("/bin/bash", abs_exec.c_str());
+
+    // Test absolute executable w/ args
+    abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "/bin/bash arg \"arg 2\" arg' '3 arg\\ 4");
+    EXPECT_STREQ("/bin/bash arg \"arg 2\" \"arg 3\" \"arg 4\"", abs_exec.c_str());
+
+    // Test relative executable (w/ ./)
+    abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "./Utils_test");
+    EXPECT_TRUE(boost::filesystem::exists(abs_exec));
+
+    // Test relative executable (w/o ./)
+    abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "Utils_test");
+    EXPECT_TRUE(boost::filesystem::exists(abs_exec));
+}

=== modified file 'test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp'
--- test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp	2015-07-22 03:54:35 +0000
+++ test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp	2015-09-15 07:36:31 +0000
@@ -28,6 +28,7 @@
 #include <unity/scopes/internal/ScopeMetadataImpl.h>
 #include <unity/scopes/internal/ScopeImpl.h>
 #include <unity/scopes/internal/UniqueID.h>
+#include <unity/scopes/internal/Utils.h>
 #include <unity/scopes/internal/zmq_middleware/ZmqRegistry.h>
 #include <unity/scopes/SearchMetadata.h>
 #include <unity/scopes/ScopeExceptions.h>
@@ -465,42 +466,6 @@
         return count_child_procs() - start_process_count;
     }
 
-    string convert_custom_exec(fs::path const& scope_dir, string const& custom_exec)
-    {
-        string result;
-
-        vector<string> split;
-        boost::split(split, custom_exec, boost::is_space());
-        fs::path program(split.front());
-        if (program.is_relative())
-        {
-            // First look inside the arch-specific directory
-            if (fs::exists(scope_dir / DEB_HOST_MULTIARCH / program))
-            {
-                // Join the full command, not just the program path
-                result = (scope_dir / DEB_HOST_MULTIARCH / custom_exec).native();
-            }
-            // Next try in the non arch-aware directory
-            else if (fs::exists(scope_dir / program))
-            {
-                // Join the full command, not just the program path
-                result = (scope_dir / custom_exec).native();
-            }
-            else
-            {
-                throw unity::InvalidArgumentException(
-                        "Nonexistent scope runner '" + custom_exec
-                                + "' executable for scope");
-            }
-        }
-        else
-        {
-            result = custom_exec;
-        }
-
-        return result;
-    }
-
     ScopeProxy start_testscopeB()
     {
         std::string test_scope_id = "testscopeB";
@@ -519,7 +484,7 @@
 
         RegistryObject::ScopeExecData exec_data;
         exec_data.scope_id = test_scope_id;
-        exec_data.custom_exec = convert_custom_exec(REGISTRY_TEST_DIR "/scopes/testscopeB", sc.scope_runner());
+        exec_data.custom_exec = convert_exec_rel_to_abs(test_scope_id, REGISTRY_TEST_DIR "/scopes/testscopeB", sc.scope_runner());
         exec_data.runtime_config = runtime_ini;
         exec_data.scope_config = test_scope_config;
         exec_data.timeout_ms = process_timeout;

=== modified file 'unity-scopes.map'
--- unity-scopes.map	2015-04-10 03:55:25 +0000
+++ unity-scopes.map	2015-09-15 07:36:31 +0000
@@ -13,6 +13,7 @@
         virtual?thunk?to?unity::scopes::[!iu]*;
         vtable?for?unity::scopes::utility::[!i]*;
         vtable?for?unity::scopes::[!iu]*;
+        unity::scopes::internal::convert_exec_rel_to_abs*;
         unity::scopes::internal::Executor::*;
         unity::scopes::internal::IniSettingsSchema::*;
         unity::scopes::internal::JsonSettingsSchema::*;