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

Use command argument in Image.show #4695

Closed
wants to merge 2 commits into from

Conversation

radarhere
Copy link
Member

@radarhere radarhere commented Jun 14, 2020

See #4643

The issue reports that the command argument is not used by Image show(). It should be a way to provide a custom command to use on the command line to show the image. PR #4646 suggests deprecating it as a solution.

As an alternative, this PR actually implements the command argument.

@hugovk
Copy link
Member

hugovk commented Jun 14, 2020

Just a thought: the test uses a command to delete a file.

Are we opening up a potential security hole, and might it be better to leave this broken and deprecate and eventually remove instead?

@radarhere
Copy link
Member Author

You can use subprocess.Popen instead, to ensure that you're just running a single executable with the image file. I just can't find a way to successfully test that.

But one might still think that any form of the command argument - allowing an arbitrary executable to be run - is still unusual behaviour from a security perspective. So sure, this can be closed if deprecation is the agreed solution.

@nulano
Copy link
Contributor

nulano commented Jun 15, 2020

Using subprocess.Popen doesn't allow for passing arguments to the running executable with command=, does it? I'm sure there are viewers where that could be necessary.

The reason I don't like the command argument is that it seems to be just a generic way to ask Pillow to run an arbitrary command on the file. If that is the intended behaviour, why not just call the relevant function exec or something similar? And that obviously sounds like a potential security issue.

I don't think registering a custom Viewer with ImageShow.register is too much to ask, especially if the documentation is improved (e.g. #4697); the one in this PR is only 3-4 lines anyway.

The only reason command would be useful is to select a viewer out of the available viewers, but arbitrary options can already be passed to custom viewers to filter them by calling ImageShow.show(image, **extra_args), or XYZViewer.show(image) can be called directly.

@hugovk
Copy link
Member

hugovk commented Jun 20, 2020

I'm in favour of deprecating rather than fixing for security reasons: we have an alternative, it's not been working for a long time (~decades?), and no-one has reported it until now.

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.

3 participants