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
=== 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 @@
22#include <string>22#include <string>
23#include <sstream>23#include <sstream>
2424
25#include <boost/filesystem.hpp>
26
25namespace unity27namespace unity
26{28{
2729
@@ -57,6 +59,9 @@
5759
58void make_directories(std::string const& path_name, mode_t mode);60void make_directories(std::string const& path_name, mode_t mode);
5961
62std::vector<std::string> split_exec_args(std::string const& id, std::string const& custom_exec);
63std::string convert_exec_rel_to_abs(std::string const& id, boost::filesystem::path const& scope_dir, std::string const& custom_exec);
64
60} // namespace internal65} // namespace internal
6166
62} // namespace scopes67} // namespace scopes
6368
=== 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 @@
259 }259 }
260}260}
261261
262string get_first_component(string const& id, string const& custom_exec)
263{
264 if (custom_exec.empty())
265 {
266 throw unity::InvalidArgumentException("Invalid scope runner executable for scope: " + id);
267 }
268
269 wordexp_t exp;
270 string result;
271
272 // Split command into program and args
273 if (wordexp(custom_exec.c_str(), &exp, WRDE_NOCMD) == 0)
274 {
275 util::ResourcePtr<wordexp_t*, decltype(&wordfree)> free_guard(&exp, wordfree);
276 result = exp.we_wordv[0];
277 }
278 else
279 {
280 throw unity::InvalidArgumentException("Invalid scope runner executable for scope: " + id);
281 }
282
283 return result;
284}
285
286// For each scope, open the config file for the scope, create the metadata info from the config,262// For each scope, open the config file for the scope, create the metadata info from the config,
287// and add an entry to the RegistryObject.263// and add an entry to the RegistryObject.
288// If the scope uses settings, also parse the settings file and add the settings to the metadata.264// If the scope uses settings, also parse the settings file and add the settings to the metadata.
@@ -435,33 +411,7 @@
435411
436 try412 try
437 {413 {
438 auto custom_exec = sc.scope_runner();414 exec_data.custom_exec = convert_exec_rel_to_abs(scope.first, scope_dir, sc.scope_runner());
439 filesystem::path program(get_first_component( scope.first, custom_exec));
440 if (program.is_relative())
441 {
442 // First look inside the arch-specific directory
443 if (filesystem::exists(scope_dir / DEB_HOST_MULTIARCH / program))
444 {
445 // Join the full command, not just the program path
446 exec_data.custom_exec = (scope_dir / DEB_HOST_MULTIARCH / custom_exec).native();
447 }
448 // Next try in the non arch-aware directory
449 else if (filesystem::exists(scope_dir / program))
450 {
451 // Join the full command, not just the program path
452 exec_data.custom_exec = (scope_dir / custom_exec).native();
453 }
454 else
455 {
456 throw unity::InvalidArgumentException(
457 "Nonexistent scope runner '" + custom_exec
458 + "' executable for scope: " + scope.first);
459 }
460 }
461 else
462 {
463 exec_data.custom_exec = custom_exec;
464 }
465 }415 }
466 catch (NotFoundException const&)416 catch (NotFoundException const&)
467 {417 {
468418
=== 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 @@
19#include <unity/scopes/internal/Utils.h>19#include <unity/scopes/internal/Utils.h>
20#include <unity/scopes/ScopeExceptions.h>20#include <unity/scopes/ScopeExceptions.h>
21#include <unity/UnityExceptions.h>21#include <unity/UnityExceptions.h>
22#include <unity/util/ResourcePtr.h>
2223
23#include <boost/filesystem.hpp>24#include <boost/filesystem.hpp>
2425
25#include <iomanip>26#include <iomanip>
26#include <locale>27#include <locale>
27#include <mutex>28#include <mutex>
29#include <wordexp.h>
2830
29#include <sys/stat.h>31#include <sys/stat.h>
3032
@@ -190,6 +192,87 @@
190 }192 }
191}193}
192194
195vector<string> split_exec_args(string const& id, string const& custom_exec)
196{
197 if (custom_exec.empty())
198 {
199 throw unity::InvalidArgumentException("Invalid empty executable for scope: '" + id + "'");
200 }
201
202 wordexp_t exp;
203 vector<string> result;
204
205 // Split command into program and args
206 if (wordexp(custom_exec.c_str(), &exp, WRDE_NOCMD) == 0)
207 {
208 util::ResourcePtr<wordexp_t*, decltype(&wordfree)> free_guard(&exp, wordfree);
209 for (size_t i = 0; i < exp.we_wordc; ++i )
210 {
211 auto argument = string(exp.we_wordv[i]);
212 if(argument.find_first_of(' ') != std::string::npos)
213 {
214 // This argument contains spaces, enclose it in quotation marks
215 result.emplace_back("\"" + argument + "\"");
216 }
217 else
218 {
219 result.emplace_back(argument);
220 }
221 }
222 }
223 else
224 {
225 throw unity::InvalidArgumentException("Invalid executable for scope: '" + id + "'");
226 }
227
228 return result;
229}
230
231string convert_exec_rel_to_abs(string const& id, boost::filesystem::path const& scope_dir, string const& custom_exec)
232{
233 string result;
234
235 // Loop through each argument of the scope runner command and ensure all path args are absolute
236 auto custom_exec_args = split_exec_args(id, custom_exec);
237 for (auto custom_exec_arg : custom_exec_args)
238 {
239 boost::filesystem::path argument(custom_exec_arg);
240 if (argument.is_relative())
241 {
242 // First look inside the arch-specific directory
243 if (boost::filesystem::exists(scope_dir / DEB_HOST_MULTIARCH / argument))
244 {
245 // Append the argument as a relative path
246 result += (scope_dir / DEB_HOST_MULTIARCH / argument).native() + " ";
247 }
248 // Next try in the non arch-aware directory
249 else if (boost::filesystem::exists(scope_dir / argument))
250 {
251 // Append the argument as a relative path
252 result += (scope_dir / argument).native() + " ";
253 }
254 // If this is the first argument (program name) it must exist, throw here
255 else if (result.empty())
256 {
257 throw unity::InvalidArgumentException(
258 "Nonexistent scope runner executable: '" + custom_exec_arg
259 + "' for scope: '" + id + "'");
260 }
261 // Otherwise just append the argument as is
262 else
263 {
264 result += custom_exec_arg + " ";
265 }
266 }
267 else
268 {
269 result += custom_exec_arg + " ";
270 }
271 }
272 result.resize(result.size() - 1);
273 return result;
274}
275
193} // namespace internal276} // namespace internal
194277
195} // namespace scopes278} // namespace scopes
196279
=== 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 @@
25#include <unity/scopes/Runtime.h>25#include <unity/scopes/Runtime.h>
26#include <unity/scopes/PreviewReply.h>26#include <unity/scopes/PreviewReply.h>
2727
28#include <boost/filesystem.hpp>
29
30using namespace boost;
28using namespace std;31using namespace std;
29using namespace unity::scopes;32using namespace unity::scopes;
3033
@@ -102,10 +105,11 @@
102 std::string scope_config = argc > 2 ? argv[2] : "FAIL";105 std::string scope_config = argc > 2 ? argv[2] : "FAIL";
103 std::string rt_config = argc > 3 ? argv[3] : "FAIL";106 std::string rt_config = argc > 3 ? argv[3] : "FAIL";
104 std::string unused_arg_2 = argc > 4 ? argv[4] : "FAIL";107 std::string unused_arg_2 = argc > 4 ? argv[4] : "FAIL";
108 filesystem::path abs_path_arg(argc > 5 ? argv[5] : "FAIL");
105109
106 // In order to test that custom exec splitting works, we check here that our arbitrary arguments110 // In order to test that custom exec splitting works, we check here that our arbitrary arguments
107 // have been delivered to us as expected.111 // have been delivered to us as expected.
108 if (unused_arg_1 == "unused arg 1" && unused_arg_2 == "unused arg 2")112 if (unused_arg_1 == "unused arg 1" && unused_arg_2 == "unused arg 2" && filesystem::exists(abs_path_arg))
109 {113 {
110 MyScope scope;114 MyScope scope;
111 auto runtime = Runtime::create_scope_runtime("testscopeB", rt_config);115 auto runtime = Runtime::create_scope_runtime("testscopeB", rt_config);
112116
=== 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 @@
6Icon = data/scope-B.Icon6Icon = data/scope-B.Icon
7SearchHint = scope-B.SearchHint7SearchHint = scope-B.SearchHint
8HotKey = scope-B.HotKey8HotKey = scope-B.HotKey
9ScopeRunner = testscopeB "unused arg 1" %S %R unused' 'arg' '29ScopeRunner = testscopeB "unused arg 1" %S %R unused' 'arg' '2 ./testscopeB.ini
10IdleTimeout = 210IdleTimeout = 2
1111
=== 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 @@
132132
133 chmod(child.c_str(), 0777); // Don't leave the dir behind without permissions.133 chmod(child.c_str(), 0777); // Don't leave the dir behind without permissions.
134}134}
135
136TEST(Utils, split_exec_args)
137{
138 // Test empty executable
139 try
140 {
141 split_exec_args("test", "");
142 FAIL();
143 }
144 catch (std::exception const& e)
145 {
146 EXPECT_STREQ("unity::InvalidArgumentException: Invalid empty executable for scope: 'test'", e.what());
147 }
148
149 // Test invalid executable
150 try
151 {
152 split_exec_args("test", "\"");
153 FAIL();
154 }
155 catch (std::exception const& e)
156 {
157 EXPECT_STREQ("unity::InvalidArgumentException: Invalid executable for scope: 'test'", e.what());
158 }
159
160 // Test argument splitting
161 auto exec_args = split_exec_args("test", "/path\\ to/exec' 'file arg \"arg 2\" arg' '3 arg\\ 4");
162 ASSERT_EQ(5, exec_args.size());
163 EXPECT_STREQ("\"/path to/exec file\"", exec_args[0].c_str());
164 EXPECT_STREQ("arg", exec_args[1].c_str());
165 EXPECT_STREQ("\"arg 2\"", exec_args[2].c_str());
166 EXPECT_STREQ("\"arg 3\"", exec_args[3].c_str());
167 EXPECT_STREQ("\"arg 4\"", exec_args[4].c_str());
168}
169
170TEST(Utils, convert_exec_rel_to_abs)
171{
172 // Test empty executable
173 try
174 {
175 convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "");
176 FAIL();
177 }
178 catch (std::exception const& e)
179 {
180 EXPECT_STREQ("unity::InvalidArgumentException: Invalid empty executable for scope: 'test'", e.what());
181 }
182
183 // Test invalid executable
184 try
185 {
186 convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "\"");
187 FAIL();
188 }
189 catch (std::exception const& e)
190 {
191 EXPECT_STREQ("unity::InvalidArgumentException: Invalid executable for scope: 'test'", e.what());
192 }
193
194 // Test nonexistent executable
195 try
196 {
197 convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "noexec");
198 FAIL();
199 }
200 catch (std::exception const& e)
201 {
202 EXPECT_STREQ("unity::InvalidArgumentException: Nonexistent scope runner executable: 'noexec' for scope: 'test'", e.what());
203 }
204
205 // Test absolute executable
206 auto abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "/bin/bash");
207 EXPECT_STREQ("/bin/bash", abs_exec.c_str());
208
209 // Test absolute executable w/ args
210 abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "/bin/bash arg \"arg 2\" arg' '3 arg\\ 4");
211 EXPECT_STREQ("/bin/bash arg \"arg 2\" \"arg 3\" \"arg 4\"", abs_exec.c_str());
212
213 // Test relative executable (w/ ./)
214 abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "./Utils_test");
215 EXPECT_TRUE(boost::filesystem::exists(abs_exec));
216
217 // Test relative executable (w/o ./)
218 abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "Utils_test");
219 EXPECT_TRUE(boost::filesystem::exists(abs_exec));
220}
135221
=== 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 @@
28#include <unity/scopes/internal/ScopeMetadataImpl.h>28#include <unity/scopes/internal/ScopeMetadataImpl.h>
29#include <unity/scopes/internal/ScopeImpl.h>29#include <unity/scopes/internal/ScopeImpl.h>
30#include <unity/scopes/internal/UniqueID.h>30#include <unity/scopes/internal/UniqueID.h>
31#include <unity/scopes/internal/Utils.h>
31#include <unity/scopes/internal/zmq_middleware/ZmqRegistry.h>32#include <unity/scopes/internal/zmq_middleware/ZmqRegistry.h>
32#include <unity/scopes/SearchMetadata.h>33#include <unity/scopes/SearchMetadata.h>
33#include <unity/scopes/ScopeExceptions.h>34#include <unity/scopes/ScopeExceptions.h>
@@ -465,42 +466,6 @@
465 return count_child_procs() - start_process_count;466 return count_child_procs() - start_process_count;
466 }467 }
467468
468 string convert_custom_exec(fs::path const& scope_dir, string const& custom_exec)
469 {
470 string result;
471
472 vector<string> split;
473 boost::split(split, custom_exec, boost::is_space());
474 fs::path program(split.front());
475 if (program.is_relative())
476 {
477 // First look inside the arch-specific directory
478 if (fs::exists(scope_dir / DEB_HOST_MULTIARCH / program))
479 {
480 // Join the full command, not just the program path
481 result = (scope_dir / DEB_HOST_MULTIARCH / custom_exec).native();
482 }
483 // Next try in the non arch-aware directory
484 else if (fs::exists(scope_dir / program))
485 {
486 // Join the full command, not just the program path
487 result = (scope_dir / custom_exec).native();
488 }
489 else
490 {
491 throw unity::InvalidArgumentException(
492 "Nonexistent scope runner '" + custom_exec
493 + "' executable for scope");
494 }
495 }
496 else
497 {
498 result = custom_exec;
499 }
500
501 return result;
502 }
503
504 ScopeProxy start_testscopeB()469 ScopeProxy start_testscopeB()
505 {470 {
506 std::string test_scope_id = "testscopeB";471 std::string test_scope_id = "testscopeB";
@@ -519,7 +484,7 @@
519484
520 RegistryObject::ScopeExecData exec_data;485 RegistryObject::ScopeExecData exec_data;
521 exec_data.scope_id = test_scope_id;486 exec_data.scope_id = test_scope_id;
522 exec_data.custom_exec = convert_custom_exec(REGISTRY_TEST_DIR "/scopes/testscopeB", sc.scope_runner());487 exec_data.custom_exec = convert_exec_rel_to_abs(test_scope_id, REGISTRY_TEST_DIR "/scopes/testscopeB", sc.scope_runner());
523 exec_data.runtime_config = runtime_ini;488 exec_data.runtime_config = runtime_ini;
524 exec_data.scope_config = test_scope_config;489 exec_data.scope_config = test_scope_config;
525 exec_data.timeout_ms = process_timeout;490 exec_data.timeout_ms = process_timeout;
526491
=== 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 @@
13 virtual?thunk?to?unity::scopes::[!iu]*;13 virtual?thunk?to?unity::scopes::[!iu]*;
14 vtable?for?unity::scopes::utility::[!i]*;14 vtable?for?unity::scopes::utility::[!i]*;
15 vtable?for?unity::scopes::[!iu]*;15 vtable?for?unity::scopes::[!iu]*;
16 unity::scopes::internal::convert_exec_rel_to_abs*;
16 unity::scopes::internal::Executor::*;17 unity::scopes::internal::Executor::*;
17 unity::scopes::internal::IniSettingsSchema::*;18 unity::scopes::internal::IniSettingsSchema::*;
18 unity::scopes::internal::JsonSettingsSchema::*;19 unity::scopes::internal::JsonSettingsSchema::*;

Subscribers

People subscribed via source and target branches

to all changes: