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.

Sign up for GitHub

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

Jump to bottom

Use a clean, whitelisted environment with Popen (#16118) #16119

Closed
wants to merge 3 commits into from
Closed

Use a clean, whitelisted environment with Popen (#16118) #16119

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efbd11d
Use a clean, whitelisted environment with Popen (#16118)
paulharris Apr 22, 2024
6cbccda
Also include conan's python path in env's PATH
paulharris Apr 22, 2024
5fe4cc9
Further hacks to test the idea
paulharris Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 58 additions & 3 deletions conans/util/runners.py
View file
Open in desktop
@@ -1,4 +1,5 @@
import os
import platform
import subprocess
import sys
import tempfile
Expand Down Expand Up @@ -32,6 +33,59 @@ def pyinstaller_bundle_env_cleaned():
yield


# Windows (and possibly other platforms) should have a clean environment,
# unpolluted by anything in the user's environment.
# Anything except the bare minimum should be added to the environment through
# conan's environment mechanisms.
# This whitelist could potentially be slimmed down further.
def _env_for_Popen():
newenv = None
if platform.system() == 'Windows':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that this sounds super risky, and it will certainly break users that depends on some of these variables for their usage.

In general it is not the responsibility of Conan to do this kind of task, it should be the users that need to spin a clean environment and then run Conan in it.
Conan has some functionality, like it can undefine/unset variables if desired in the profiles, so if you are having problems I'd recommend to define a profile that unsets the environment vars you want to filter out and then inject that profile when you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is the variables I'm having problems with are not things I'm adding. It is bash that is creating !ExitCode multiple times in different cases.
This would affect all users.

I thought it should be conan's responsibility though... I thought it was conan's job to control the build environment as best it can...

I don't think recipes should be depending on environmental variables from outside the conan-defined environment. The recipe, lock file, and profiles, should say everything.
If users want something added to their environment, add it to their profile. Isn't that what it is for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid that you are looking for a level of isolation that is outside of Conan scope. You might be able to implement that isolation with some Conan mechanisms, but it is not something that should be in the Conan scope, as Conan is a package manager not a "build environment" or a "virtualenv manager" (as a note, not even Python virtualenvs provide isolation from defined environment, if you have some env-var defined and activate a virtualenv, it doesn't provide a clean/blank environment, it inherits the env-vars already defined in the system)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll continue the discussion on the merits in the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach has been dropped in favour of clearing the env at the start of conan's execution.
More discussion in issue.

whitelist_env_keys = [
'ALLUSERSPROFILE',
'APPDATA',
'CommonProgramFiles',
'CommonProgramFiles(x86)',
'CommonProgramW6432',
'COMPUTERNAME',
'ComSpec',
'DriverData',
'HOMEDRIVE',
'HOMEPATH',
'LOCALAPPDATA',
'LOGONSERVER',
'NUMBER_OF_PROCESSORS',
'OS',
'PATHEXT',
'PROCESSOR_ARCHITECTURE',
'PROCESSOR_IDENTIFIER',
'PROCESSOR_LEVEL',
'PROCESSOR_REVISION',
'ProgramData',
'ProgramFiles',
'ProgramFiles(x86)',
'ProgramW6432',
'PUBLIC',
'SESSIONNAME',
'SystemDrive',
'SystemRoot',
'TEMP',
'TMP',
'USERDOMAIN',
'USERDOMAIN_ROAMINGPROFILE',
'USERNAME',
'USERPROFILE',
'windir'
]
newenv = { key: os.environ[key] for key in whitelist_env_keys }
# Also get the path to conan's python, as some packages assume access to python
# eg dav1d
# This will also give access to ninja.exe and whatever else is installed in the devenv
python_path = os.path.dirname(sys.executable)
newenv['PATH'] = f'C:\\WINDOWS\\system32;C:\\WINDOWS;{python_path}'
return newenv


def conan_run(command, stdout=None, stderr=None, cwd=None, shell=True):
"""
@param shell:
Expand All @@ -48,7 +102,7 @@ def conan_run(command, stdout=None, stderr=None, cwd=None, shell=True):

with pyinstaller_bundle_env_cleaned():
try:
proc = subprocess.Popen(command, shell=shell, stdout=out, stderr=err, cwd=cwd)
proc = subprocess.Popen(command, shell=shell, stdout=out, stderr=err, cwd=cwd, env=_env_for_Popen())
except Exception as e:
raise ConanException("Error while running cmd\nError: %s" % (str(e)))

Expand All @@ -65,7 +119,8 @@ def conan_run(command, stdout=None, stderr=None, cwd=None, shell=True):
def detect_runner(command):
# Running detect.py automatic detection of profile
proc = subprocess.Popen(command, shell=True, bufsize=1, universal_newlines=True,
stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
env=_env_for_Popen())

output_buffer = []
while True:
Expand All @@ -88,7 +143,7 @@ def check_output_runner(cmd, stderr=None, ignore_error=False):
# We don't want stderr to print warnings that will mess the pristine outputs
stderr = stderr or subprocess.PIPE
command = '{} > "{}"'.format(cmd, tmp_file)
process = subprocess.Popen(command, shell=True, stderr=stderr)
process = subprocess.Popen(command, shell=True, stderr=stderr, env=_env_for_Popen())
stdout, stderr = process.communicate()

if process.returncode and not ignore_error:
Expand Down