Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid need to quote arguments for commands running without a shell (#5578) #5583

Merged
merged 7 commits into from Jun 4, 2020
Merged

Avoid need to quote arguments for commands running without a shell (#5578) #5583

merged 7 commits into from Jun 4, 2020

Conversation

jkuebart
Copy link
Contributor

@jkuebart jkuebart commented Aug 5, 2019

Changelog: Feature: Pass command to Runner as a sequence instead of string.
Docs: conan-io/docs#1385

Closes #5578

ConanFile's run() method only support commands passed as strings. This forces users to correctly quote all arguments diligently, which is not only a frequent source of bugs but also leaves naïve but innocent-looking recipes wide open to code-injection bugs of the form ; rm -rf ~;.

The underlying Python call subprocess.Popen() already supports this, and os.system() can be replaced by subprocess.run() which does this as well.

This PR seamlessly supports commands as both strings and sequences and is fully backwards-compatible. It provides a huge improvement to Conan's usability and security.

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Aug 5, 2019

CLA assistant check
All committers have signed the CLA.

@sarikakaran sarikakaran added this to To Do in Test Aug 5, 2019
@sarikakaran sarikakaran removed this from To Do in Test Aug 5, 2019
Copy link
Member

@memsharded memsharded left a comment

Hi @jkuebart

Thanks very much for your pull request.

I think that it might be doing some things besides what the title says: "specify commands as sequences", in this case, the changes to the environment, that might not be necessary. Are they really necessary for being able to pass the command as a sequence?

Please have a look to the comments, specially regarding possible breaking behaviors. While we try to have good test coverage, there might always be cases that are not, so please consider those comments.

Also, have a look to the broken tests in https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-5583/1/pipeline, try running them locally, I have had a look at CI, but not evident for me what has changed. Thanks!

conans/client/runner.py Outdated Show resolved Hide resolved
conans/model/conan_file.py Outdated Show resolved Hide resolved
@jkuebart
Copy link
Contributor Author

@jkuebart jkuebart commented Aug 6, 2019

Hi James,

thank you for your comments and taking the time to look at this.

I think that it might be doing some things besides what the title says: "specify commands as sequences", in this case, the changes to the environment, that might not be necessary. Are they really necessary for being able to pass the command as a sequence?

You are right, previously Conan modified its own process environment before executing children and restored it later. I changed it such that the desired environment is instead passed to Popen()/subprocess.call().

I originally made this change because when passing the command as a sequence it's not possible to simply add a variable as was done by the code in the case of DYLD_LIBRARY_PATH. However, I now understand that this is only required when running through the shell, so I'll have a look to put the code back to the way it was beforehand.

In the long run, I believe that specifying the environment on execution instead of modifying the own process environment is slightly nicer, but that can be discussed separately.

Also, have a look to the broken tests in https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-5583/1/pipeline, try running them locally, I have had a look at CI, but not evident for me what has changed. Thanks!

I'm looking at the CI now, some environment variables seem to get expanded when previously they didn't – I haven't understood what caused this yet…

@jkuebart jkuebart changed the title Support running commands specified as a sequences (#5578) Support running commands specified as sequences (#5578) Aug 7, 2019
@jkuebart
Copy link
Contributor Author

@jkuebart jkuebart commented Aug 8, 2019

@memsharded Hi James, I made the changes you suggested and the builds are now passing :-)

Copy link
Member

@memsharded memsharded left a comment

Thanks very much for taking into account the review.
I'd like to know better about the shell features, please check comments.

conans/client/runner.py Show resolved Hide resolved
conans/model/conan_file.py Show resolved Hide resolved
@jkuebart
Copy link
Contributor Author

@jkuebart jkuebart commented Aug 16, 2019

@memsharded Hi James, have you had a chance to have another look at this patch?

@jkuebart
Copy link
Contributor Author

@jkuebart jkuebart commented Nov 15, 2019

@uilianries updated – would love to hear your comments.

@jkuebart jkuebart changed the title Support running commands specified as sequences (#5578) Avoid need to quote arguments for commands running without a shell (#5578) Nov 21, 2019
@jkuebart jkuebart requested a review from memsharded Nov 21, 2019
@jkuebart
Copy link
Contributor Author

@jkuebart jkuebart commented Mar 10, 2020

I think it's really a shame that a security-critical PR like this remains ignored.

Requested changes were applied, re-review requested, all tests are passing and for all intents and purposes the change is both backwards compatible and in line with the functionality of the underlying Python library.

As mentioned in the initial comment, this solves security-critical problems that come up all the time in a clean manner which could be used immediately by new and existing projects. Still, there is no feedback on this for six months without any way to know what is holding up progress.

@memsharded memsharded added this to the 1.26 milestone May 6, 2020
Copy link
Member

@jgsogo jgsogo left a comment

I'd like to move forward this PR.


Maybe we can even consider for the CONAN_V2_MODE to run these commands with shell=False

conans/client/runner.py Show resolved Hide resolved
conans/model/conan_file.py Show resolved Hide resolved
@jkuebart
Copy link
Contributor Author

@jkuebart jkuebart commented May 27, 2020

I'd like to move forward this PR.

@jgsogo that's great, thanks for taking this up! I've merged to develop to make sure tests are still passing.

Maybe we can even consider for the CONAN_V2_MODE to run these commands with shell=False

I think being able to run in a shell can be a useful feature – it makes it easy to reuse some features that are easy to do in a shell. Whether it should be provided implicitly (by passing strings vs arrays) or explicitly is of course debatable.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Jun 3, 2020

I've created a PR adding a test here https://github.com/jkuebart/conan/pull/1

Please, @memsharded , let me know if we need any other test besides that one, and please @jkuebart merge that PR if you see it is correct (if not, tomorrow we will add it to your branch because we want to finish the release asap).

Thanks!

jgsogo
jgsogo approved these changes Jun 4, 2020
@memsharded memsharded merged commit 0eea418 into conan-io:develop Jun 4, 2020
2 checks passed
@jkuebart
Copy link
Contributor Author

@jkuebart jkuebart commented Jun 5, 2020

Hi @jsgogo, thank you for getting this merged so quickly in the end.

I've just verified the test you added by removing the section in conans/model/conan_file.py that passes DYLD_LIBRARY_PATH through the shell and I'm getting the expected failure:

------------------------ Command failed (unexpectedly): ------------------------
build .
----------------------------------- Output: ------------------------------------
Using lockfile: '/private/var/folders/bn/75_018nd17sc69hpp_wk20qr0000gn/T/tmp4aitz14pconans/path with spaces/conan.lock'
Using cached profile from lockfile
conanfile.py: Calling build()

----Running------
> say_hello
-----------------
dyld: Library not loaded: /private/var/folders/bn/75_018nd17sc69hpp_wk20qr0000gn/T/tmpi6p7rd3yconans/path with spaces/data/Pkg/0.1/lasote/testing/build/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/libhello.dylib
  Referenced from: /private/var/folders/bn/75_018nd17sc69hpp_wk20qr0000gn/T/tmpkngivvd1conans/path with spaces/data/Pkg/0.1/lasote/testing/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/bin/say_hello
  Reason: image not found
ERROR: conanfile.py: Error in build() method, line 12
	self.run("say_hello", run_environment=True)
	ConanException: Error -6 while executing say_hello

Normally the test passes, proving that it works and in fact tests what it's supposed to test.

@memsharded
Copy link
Member

@memsharded memsharded commented Jun 5, 2020

yeah, good job! Sorry @jkuebart that this took so long to process, sometimes issues and contributions get buried in the backlog, the activity is so high that is inevitable that some things fall through the cracks. Thanks for bringing attention to this again.

@jkuebart jkuebart deleted the feature/command-sequence branch Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants