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

Proposed by Marcus Tomlinson on 2015-09-11
Status: Merged
Approved by: Paweł Stołowski on 2015-09-15
Approved revision: 631
Merged at revision: 631
Proposed branch: lp:~marcustomlinson/unity-scopes-api/fix_relative_custom_exec_args
Merge into: lp:unity-scopes-api/devel
Diff against target: 407 lines (+184/-90)
8 files modified
include/unity/scopes/internal/Utils.h (+5/-0)
scoperegistry/scoperegistry.cpp (+1/-51)
src/scopes/internal/Utils.cpp (+83/-0)
test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp (+5/-1)
test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in (+1/-1)
test/gtest/scopes/internal/Utils/Utils_test.cpp (+86/-0)
test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp (+2/-37)
unity-scopes.map (+1/-0)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scopes-api/fix_relative_custom_exec_args
Reviewer Review Type Date Requested Status
Paweł Stołowski 2015-09-11 Approve on 2015-09-15
PS Jenkins bot (community) continuous-integration Approve on 2015-09-15
Review via email: mp+270777@code.launchpad.net

Commit message

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

To post a comment you must log in.
Paweł Stołowski (stolowski) wrote :

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
631. By Marcus Tomlinson on 2015-09-15

Added tests

Marcus Tomlinson (marcustomlinson) wrote :

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

Paweł Stołowski (stolowski) wrote :

Awesome, thanks! +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/unity/scopes/internal/Utils.h'
2--- include/unity/scopes/internal/Utils.h 2015-04-10 03:55:25 +0000
3+++ include/unity/scopes/internal/Utils.h 2015-09-15 07:36:31 +0000
4@@ -22,6 +22,8 @@
5 #include <string>
6 #include <sstream>
7
8+#include <boost/filesystem.hpp>
9+
10 namespace unity
11 {
12
13@@ -57,6 +59,9 @@
14
15 void make_directories(std::string const& path_name, mode_t mode);
16
17+std::vector<std::string> split_exec_args(std::string const& id, std::string const& custom_exec);
18+std::string convert_exec_rel_to_abs(std::string const& id, boost::filesystem::path const& scope_dir, std::string const& custom_exec);
19+
20 } // namespace internal
21
22 } // namespace scopes
23
24=== modified file 'scoperegistry/scoperegistry.cpp'
25--- scoperegistry/scoperegistry.cpp 2015-07-01 11:02:46 +0000
26+++ scoperegistry/scoperegistry.cpp 2015-09-15 07:36:31 +0000
27@@ -259,30 +259,6 @@
28 }
29 }
30
31-string get_first_component(string const& id, string const& custom_exec)
32-{
33- if (custom_exec.empty())
34- {
35- throw unity::InvalidArgumentException("Invalid scope runner executable for scope: " + id);
36- }
37-
38- wordexp_t exp;
39- string result;
40-
41- // Split command into program and args
42- if (wordexp(custom_exec.c_str(), &exp, WRDE_NOCMD) == 0)
43- {
44- util::ResourcePtr<wordexp_t*, decltype(&wordfree)> free_guard(&exp, wordfree);
45- result = exp.we_wordv[0];
46- }
47- else
48- {
49- throw unity::InvalidArgumentException("Invalid scope runner executable for scope: " + id);
50- }
51-
52- return result;
53-}
54-
55 // For each scope, open the config file for the scope, create the metadata info from the config,
56 // and add an entry to the RegistryObject.
57 // If the scope uses settings, also parse the settings file and add the settings to the metadata.
58@@ -435,33 +411,7 @@
59
60 try
61 {
62- auto custom_exec = sc.scope_runner();
63- filesystem::path program(get_first_component( scope.first, custom_exec));
64- if (program.is_relative())
65- {
66- // First look inside the arch-specific directory
67- if (filesystem::exists(scope_dir / DEB_HOST_MULTIARCH / program))
68- {
69- // Join the full command, not just the program path
70- exec_data.custom_exec = (scope_dir / DEB_HOST_MULTIARCH / custom_exec).native();
71- }
72- // Next try in the non arch-aware directory
73- else if (filesystem::exists(scope_dir / program))
74- {
75- // Join the full command, not just the program path
76- exec_data.custom_exec = (scope_dir / custom_exec).native();
77- }
78- else
79- {
80- throw unity::InvalidArgumentException(
81- "Nonexistent scope runner '" + custom_exec
82- + "' executable for scope: " + scope.first);
83- }
84- }
85- else
86- {
87- exec_data.custom_exec = custom_exec;
88- }
89+ exec_data.custom_exec = convert_exec_rel_to_abs(scope.first, scope_dir, sc.scope_runner());
90 }
91 catch (NotFoundException const&)
92 {
93
94=== modified file 'src/scopes/internal/Utils.cpp'
95--- src/scopes/internal/Utils.cpp 2015-04-10 03:55:25 +0000
96+++ src/scopes/internal/Utils.cpp 2015-09-15 07:36:31 +0000
97@@ -19,12 +19,14 @@
98 #include <unity/scopes/internal/Utils.h>
99 #include <unity/scopes/ScopeExceptions.h>
100 #include <unity/UnityExceptions.h>
101+#include <unity/util/ResourcePtr.h>
102
103 #include <boost/filesystem.hpp>
104
105 #include <iomanip>
106 #include <locale>
107 #include <mutex>
108+#include <wordexp.h>
109
110 #include <sys/stat.h>
111
112@@ -190,6 +192,87 @@
113 }
114 }
115
116+vector<string> split_exec_args(string const& id, string const& custom_exec)
117+{
118+ if (custom_exec.empty())
119+ {
120+ throw unity::InvalidArgumentException("Invalid empty executable for scope: '" + id + "'");
121+ }
122+
123+ wordexp_t exp;
124+ vector<string> result;
125+
126+ // Split command into program and args
127+ if (wordexp(custom_exec.c_str(), &exp, WRDE_NOCMD) == 0)
128+ {
129+ util::ResourcePtr<wordexp_t*, decltype(&wordfree)> free_guard(&exp, wordfree);
130+ for (size_t i = 0; i < exp.we_wordc; ++i )
131+ {
132+ auto argument = string(exp.we_wordv[i]);
133+ if(argument.find_first_of(' ') != std::string::npos)
134+ {
135+ // This argument contains spaces, enclose it in quotation marks
136+ result.emplace_back("\"" + argument + "\"");
137+ }
138+ else
139+ {
140+ result.emplace_back(argument);
141+ }
142+ }
143+ }
144+ else
145+ {
146+ throw unity::InvalidArgumentException("Invalid executable for scope: '" + id + "'");
147+ }
148+
149+ return result;
150+}
151+
152+string convert_exec_rel_to_abs(string const& id, boost::filesystem::path const& scope_dir, string const& custom_exec)
153+{
154+ string result;
155+
156+ // Loop through each argument of the scope runner command and ensure all path args are absolute
157+ auto custom_exec_args = split_exec_args(id, custom_exec);
158+ for (auto custom_exec_arg : custom_exec_args)
159+ {
160+ boost::filesystem::path argument(custom_exec_arg);
161+ if (argument.is_relative())
162+ {
163+ // First look inside the arch-specific directory
164+ if (boost::filesystem::exists(scope_dir / DEB_HOST_MULTIARCH / argument))
165+ {
166+ // Append the argument as a relative path
167+ result += (scope_dir / DEB_HOST_MULTIARCH / argument).native() + " ";
168+ }
169+ // Next try in the non arch-aware directory
170+ else if (boost::filesystem::exists(scope_dir / argument))
171+ {
172+ // Append the argument as a relative path
173+ result += (scope_dir / argument).native() + " ";
174+ }
175+ // If this is the first argument (program name) it must exist, throw here
176+ else if (result.empty())
177+ {
178+ throw unity::InvalidArgumentException(
179+ "Nonexistent scope runner executable: '" + custom_exec_arg
180+ + "' for scope: '" + id + "'");
181+ }
182+ // Otherwise just append the argument as is
183+ else
184+ {
185+ result += custom_exec_arg + " ";
186+ }
187+ }
188+ else
189+ {
190+ result += custom_exec_arg + " ";
191+ }
192+ }
193+ result.resize(result.size() - 1);
194+ return result;
195+}
196+
197 } // namespace internal
198
199 } // namespace scopes
200
201=== modified file 'test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp'
202--- test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp 2014-12-05 03:05:37 +0000
203+++ test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp 2015-09-15 07:36:31 +0000
204@@ -25,6 +25,9 @@
205 #include <unity/scopes/Runtime.h>
206 #include <unity/scopes/PreviewReply.h>
207
208+#include <boost/filesystem.hpp>
209+
210+using namespace boost;
211 using namespace std;
212 using namespace unity::scopes;
213
214@@ -102,10 +105,11 @@
215 std::string scope_config = argc > 2 ? argv[2] : "FAIL";
216 std::string rt_config = argc > 3 ? argv[3] : "FAIL";
217 std::string unused_arg_2 = argc > 4 ? argv[4] : "FAIL";
218+ filesystem::path abs_path_arg(argc > 5 ? argv[5] : "FAIL");
219
220 // In order to test that custom exec splitting works, we check here that our arbitrary arguments
221 // have been delivered to us as expected.
222- if (unused_arg_1 == "unused arg 1" && unused_arg_2 == "unused arg 2")
223+ if (unused_arg_1 == "unused arg 1" && unused_arg_2 == "unused arg 2" && filesystem::exists(abs_path_arg))
224 {
225 MyScope scope;
226 auto runtime = Runtime::create_scope_runtime("testscopeB", rt_config);
227
228=== modified file 'test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in'
229--- test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in 2014-12-08 15:17:04 +0000
230+++ test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in 2015-09-15 07:36:31 +0000
231@@ -6,5 +6,5 @@
232 Icon = data/scope-B.Icon
233 SearchHint = scope-B.SearchHint
234 HotKey = scope-B.HotKey
235-ScopeRunner = testscopeB "unused arg 1" %S %R unused' 'arg' '2
236+ScopeRunner = testscopeB "unused arg 1" %S %R unused' 'arg' '2 ./testscopeB.ini
237 IdleTimeout = 2
238
239=== modified file 'test/gtest/scopes/internal/Utils/Utils_test.cpp'
240--- test/gtest/scopes/internal/Utils/Utils_test.cpp 2015-04-10 03:55:25 +0000
241+++ test/gtest/scopes/internal/Utils/Utils_test.cpp 2015-09-15 07:36:31 +0000
242@@ -132,3 +132,89 @@
243
244 chmod(child.c_str(), 0777); // Don't leave the dir behind without permissions.
245 }
246+
247+TEST(Utils, split_exec_args)
248+{
249+ // Test empty executable
250+ try
251+ {
252+ split_exec_args("test", "");
253+ FAIL();
254+ }
255+ catch (std::exception const& e)
256+ {
257+ EXPECT_STREQ("unity::InvalidArgumentException: Invalid empty executable for scope: 'test'", e.what());
258+ }
259+
260+ // Test invalid executable
261+ try
262+ {
263+ split_exec_args("test", "\"");
264+ FAIL();
265+ }
266+ catch (std::exception const& e)
267+ {
268+ EXPECT_STREQ("unity::InvalidArgumentException: Invalid executable for scope: 'test'", e.what());
269+ }
270+
271+ // Test argument splitting
272+ auto exec_args = split_exec_args("test", "/path\\ to/exec' 'file arg \"arg 2\" arg' '3 arg\\ 4");
273+ ASSERT_EQ(5, exec_args.size());
274+ EXPECT_STREQ("\"/path to/exec file\"", exec_args[0].c_str());
275+ EXPECT_STREQ("arg", exec_args[1].c_str());
276+ EXPECT_STREQ("\"arg 2\"", exec_args[2].c_str());
277+ EXPECT_STREQ("\"arg 3\"", exec_args[3].c_str());
278+ EXPECT_STREQ("\"arg 4\"", exec_args[4].c_str());
279+}
280+
281+TEST(Utils, convert_exec_rel_to_abs)
282+{
283+ // Test empty executable
284+ try
285+ {
286+ convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "");
287+ FAIL();
288+ }
289+ catch (std::exception const& e)
290+ {
291+ EXPECT_STREQ("unity::InvalidArgumentException: Invalid empty executable for scope: 'test'", e.what());
292+ }
293+
294+ // Test invalid executable
295+ try
296+ {
297+ convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "\"");
298+ FAIL();
299+ }
300+ catch (std::exception const& e)
301+ {
302+ EXPECT_STREQ("unity::InvalidArgumentException: Invalid executable for scope: 'test'", e.what());
303+ }
304+
305+ // Test nonexistent executable
306+ try
307+ {
308+ convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "noexec");
309+ FAIL();
310+ }
311+ catch (std::exception const& e)
312+ {
313+ EXPECT_STREQ("unity::InvalidArgumentException: Nonexistent scope runner executable: 'noexec' for scope: 'test'", e.what());
314+ }
315+
316+ // Test absolute executable
317+ auto abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "/bin/bash");
318+ EXPECT_STREQ("/bin/bash", abs_exec.c_str());
319+
320+ // Test absolute executable w/ args
321+ abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "/bin/bash arg \"arg 2\" arg' '3 arg\\ 4");
322+ EXPECT_STREQ("/bin/bash arg \"arg 2\" \"arg 3\" \"arg 4\"", abs_exec.c_str());
323+
324+ // Test relative executable (w/ ./)
325+ abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "./Utils_test");
326+ EXPECT_TRUE(boost::filesystem::exists(abs_exec));
327+
328+ // Test relative executable (w/o ./)
329+ abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "Utils_test");
330+ EXPECT_TRUE(boost::filesystem::exists(abs_exec));
331+}
332
333=== modified file 'test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp'
334--- test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp 2015-07-22 03:54:35 +0000
335+++ test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp 2015-09-15 07:36:31 +0000
336@@ -28,6 +28,7 @@
337 #include <unity/scopes/internal/ScopeMetadataImpl.h>
338 #include <unity/scopes/internal/ScopeImpl.h>
339 #include <unity/scopes/internal/UniqueID.h>
340+#include <unity/scopes/internal/Utils.h>
341 #include <unity/scopes/internal/zmq_middleware/ZmqRegistry.h>
342 #include <unity/scopes/SearchMetadata.h>
343 #include <unity/scopes/ScopeExceptions.h>
344@@ -465,42 +466,6 @@
345 return count_child_procs() - start_process_count;
346 }
347
348- string convert_custom_exec(fs::path const& scope_dir, string const& custom_exec)
349- {
350- string result;
351-
352- vector<string> split;
353- boost::split(split, custom_exec, boost::is_space());
354- fs::path program(split.front());
355- if (program.is_relative())
356- {
357- // First look inside the arch-specific directory
358- if (fs::exists(scope_dir / DEB_HOST_MULTIARCH / program))
359- {
360- // Join the full command, not just the program path
361- result = (scope_dir / DEB_HOST_MULTIARCH / custom_exec).native();
362- }
363- // Next try in the non arch-aware directory
364- else if (fs::exists(scope_dir / program))
365- {
366- // Join the full command, not just the program path
367- result = (scope_dir / custom_exec).native();
368- }
369- else
370- {
371- throw unity::InvalidArgumentException(
372- "Nonexistent scope runner '" + custom_exec
373- + "' executable for scope");
374- }
375- }
376- else
377- {
378- result = custom_exec;
379- }
380-
381- return result;
382- }
383-
384 ScopeProxy start_testscopeB()
385 {
386 std::string test_scope_id = "testscopeB";
387@@ -519,7 +484,7 @@
388
389 RegistryObject::ScopeExecData exec_data;
390 exec_data.scope_id = test_scope_id;
391- exec_data.custom_exec = convert_custom_exec(REGISTRY_TEST_DIR "/scopes/testscopeB", sc.scope_runner());
392+ exec_data.custom_exec = convert_exec_rel_to_abs(test_scope_id, REGISTRY_TEST_DIR "/scopes/testscopeB", sc.scope_runner());
393 exec_data.runtime_config = runtime_ini;
394 exec_data.scope_config = test_scope_config;
395 exec_data.timeout_ms = process_timeout;
396
397=== modified file 'unity-scopes.map'
398--- unity-scopes.map 2015-04-10 03:55:25 +0000
399+++ unity-scopes.map 2015-09-15 07:36:31 +0000
400@@ -13,6 +13,7 @@
401 virtual?thunk?to?unity::scopes::[!iu]*;
402 vtable?for?unity::scopes::utility::[!i]*;
403 vtable?for?unity::scopes::[!iu]*;
404+ unity::scopes::internal::convert_exec_rel_to_abs*;
405 unity::scopes::internal::Executor::*;
406 unity::scopes::internal::IniSettingsSchema::*;
407 unity::scopes::internal::JsonSettingsSchema::*;

Subscribers

People subscribed via source and target branches

to all changes: