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

Open and load --code file and arguments in C++ #60134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

btzy
Copy link

@btzy btzy commented Jan 13, 2025

Description

(Note: Not yet tested yet; it seems that if I create a PR, the bot will make a Windows build for me.)

I took a stab at #60120 to see if we can design a better alternative to #60121.

Instead of doing string substitution into Python code, we use the proper Python C API functions to assign to sys.argv and execute a Python script from a file. This avoids the issues that arise due to improper escaping of strings (e.g. ' and \). Unix file names in particular may contain any non-null character (including non-printable characters), and trying to escape such characters manually would otherwise be difficult to get right. String substitution is also possibly vulnerable to code injection attacks, as it is possible to carefully craft a string containing code that will be executed.

There was a hack that converts Windows-style directory separators to Unix-style directory separators. This is no longer necessary, and therefore has been removed.

Note that this PR changes the behaviour when --py-args is specified without --code. Previously, we would set sys.argv to the arguments given, but with this PR we do not do that. As far as I can tell, --py-args is meant to be used with --code, and therefore the former was unintended.

Fixes #60120.

@domi4484
Copy link
Contributor

@btzy when we introduced --py-args we decided on purpose to make the args available for each python execution in QGIS independently from the --code argument.
But I can't remember exactly what's the use case thought, maybe @m-kuhn do you remember more?

@btzy If you need arguments valid only in the context of the --code script I would suggest to add a new parameter --code-args. But then arise the question what to do if they are both set?

@m-kuhn
Copy link
Member

m-kuhn commented Jan 16, 2025

No strong opinion. What would be the reason to not make it also available for other python code than --code?

@btzy
Copy link
Author

btzy commented Jan 17, 2025

@domi4484 I misunderstood the help string of --py-args. I'll modify this PR. Likely we'll need another function in QgsPythonUtils to set the sys.argv variable (separate from runFile).

@btzy btzy marked this pull request as ready for review January 18, 2025 15:54
@btzy
Copy link
Author

btzy commented Jan 18, 2025

I'm not sure how to trigger the automatic Windows build that other PRs have; can someone help me get the GitHub Action to run?

@btzy
Copy link
Author

btzy commented Jan 18, 2025

Separately, I made #60183 to fix the grammar error in the help, which was what originally misled me to think that --py-args is only used with --code.

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.

Windows: --code flag cannot accept paths with single quote (')
3 participants