Skip to content

Commit

Permalink
Merge pull request #51785 from arizvisa/GH-51784
Browse files Browse the repository at this point in the history 
Re-implemented salt.utils.path.which to properly define executable semantics on both the windows and posix platforms.
  • Loading branch information
@dwoz
dwoz authored Mar 12, 2019
2 parents e4df418 + aeae914 commit 7b48f54
Show file tree
Hide file tree
Showing 2 changed files with 170 additions and 115 deletions.
165 changes: 102 additions & 63 deletions salt/utils/path.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import salt.utils.platform
import salt.utils.stringutils
from salt.exceptions import CommandNotFoundError
from salt.utils.decorators import memoize as real_memoize
from salt.utils.decorators.jinja import jinja_filter

# Import 3rd-party libs
Expand Down Expand Up @@ -191,70 +190,110 @@ def which(exe=None):
'''
Python clone of /usr/bin/which
'''
def _is_executable_file_or_link(exe):
# check for os.X_OK doesn't suffice because directory may executable
return (os.access(exe, os.X_OK) and
(os.path.isfile(exe) or os.path.islink(exe)))

if exe:
if _is_executable_file_or_link(exe):
# executable in cwd or fullpath
return exe

ext_list = salt.utils.stringutils.to_str(
os.environ.get('PATHEXT', str('.EXE'))
).split(str(';'))

@real_memoize
def _exe_has_ext():
'''
Do a case insensitive test if exe has a file extension match in
PATHEXT
'''
for ext in ext_list:
try:
pattern = r'.*\.{0}$'.format(
salt.utils.stringutils.to_unicode(ext).lstrip('.')
)
re.match(
pattern,
salt.utils.stringutils.to_unicode(exe),
re.I).groups()
return True
except AttributeError:
continue
return False

# Enhance POSIX path for the reliability at some environments, when $PATH is changing
# This also keeps order, where 'first came, first win' for cases to find optional alternatives
system_path = salt.utils.stringutils.to_unicode(os.environ.get('PATH', ''))
search_path = system_path.split(os.pathsep)
if not salt.utils.platform.is_windows():
search_path.extend([
x for x in ('/bin', '/sbin', '/usr/bin',
'/usr/sbin', '/usr/local/bin')
if x not in search_path
])

for path in search_path:
full_path = join(path, exe)
if _is_executable_file_or_link(full_path):
return full_path
elif salt.utils.platform.is_windows() and not _exe_has_ext():
# On Windows, check for any extensions in PATHEXT.
# Allows both 'cmd' and 'cmd.exe' to be matched.
for ext in ext_list:
# Windows filesystem is case insensitive so we
# safely rely on that behavior
if _is_executable_file_or_link(full_path + ext):
return full_path + ext
log.trace(
'\'%s\' could not be found in the following search path: \'%s\'',
exe, search_path
)
else:

if not exe:
log.error('No executable was passed to be searched by salt.utils.path.which()')
return None

## define some utilities (we use closures here because our predecessor used them)
def is_executable_common(path):
'''
This returns truth if posixy semantics (which python simulates on
windows) states that this is executable.
'''
return os.path.isfile(path) and os.access(path, os.X_OK)

def resolve(path):
'''
This will take a path and recursively follow the link until we get to a
real file.
'''
while os.path.islink(path):
res = os.readlink(path)

# if the link points to a relative target, then convert it to an
# absolute path relative to the original path
if not os.path.isabs(res):
directory, _ = os.path.split(path)
res = join(directory, res)
path = res
return path

# windows-only
def has_executable_ext(path, ext_membership):
'''
Extract the extension from the specified path, lowercase it so we
can be insensitive, and then check it against the available exts.
'''
p, ext = os.path.splitext(path)
return ext.lower() in ext_membership

## prepare related variables from the environment
res = salt.utils.stringutils.to_unicode(os.environ.get('PATH', ''))
system_path = res.split(os.pathsep)

# add some reasonable defaults in case someone's PATH is busted
if not salt.utils.platform.is_windows():
res = set(system_path)
extended_path = ['/sbin', '/bin', '/usr/sbin', '/usr/bin', '/usr/local/sbin', '/usr/local/bin']
system_path.extend([p for p in extended_path if p not in res])

## now to define the semantics of what's considered executable on a given platform
if salt.utils.platform.is_windows():
# executable semantics on windows requires us to search PATHEXT
res = salt.utils.stringutils.to_str(os.environ.get('PATHEXT', str('.EXE')))

# generate two variables, one of them for O(n) searches (but ordered)
# and another for O(1) searches. the previous guy was trying to use
# memoization with a function that has no arguments, this provides
# the exact same benefit
pathext = res.split(os.pathsep)
res = {ext.lower() for ext in pathext}

# check if our caller already specified a valid extension as then we don't need to match it
_, ext = os.path.splitext(exe)
if ext.lower() in res:
pathext = ['']

is_executable = is_executable_common

# The specified extension isn't valid, so we just assume it's part of the
# filename and proceed to walk the pathext list
else:
is_executable = lambda path, membership=res: is_executable_common(path) and has_executable_ext(path, membership)

else:
# in posix, there's no such thing as file extensions..only zuul
pathext = ['']

# executable semantics are pretty simple on reasonable platforms...
is_executable = is_executable_common

## search for the executable

# check to see if the full path was specified as then we don't need
# to actually walk the system_path for any reason
if is_executable(exe):
return exe

# now to search through our system_path
for path in system_path:
p = join(path, exe)

# iterate through all extensions to see which one is executable
for ext in pathext:
pext = p + ext
rp = resolve(pext)
if is_executable(rp):
return p + ext
continue
continue

## if something was executable, we should've found it already...
log.trace(
'\'%s\' could not be found in the following search path: \'%s\'',
exe, system_path
)
return None


Expand Down
120 changes: 68 additions & 52 deletions tests/unit/utils/test_path.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,92 +186,108 @@ class TestWhich(TestCase):
# The mock patch below will make sure that ALL calls to the which function
# returns None
def test_missing_binary_in_linux(self):
with patch('salt.utils.path.which', lambda exe: None):
self.assertTrue(
salt.utils.path.which('this-binary-does-not-exist') is None
)
# salt.utils.path.which uses platform.is_windows to determine the platform, so we're using linux here
with patch('salt.utils.platform.is_windows', lambda: False):
with patch('salt.utils.path.which', lambda exe: None):
self.assertTrue(
salt.utils.path.which('this-binary-does-not-exist') is None
)

# The mock patch below will make sure that ALL calls to the which function
# return whatever is sent to it
def test_existing_binary_in_linux(self):
with patch('salt.utils.path.which', lambda exe: exe):
self.assertTrue(salt.utils.path.which('this-binary-exists-under-linux'))
# salt.utils.path.which uses platform.is_windows to determine the platform, so we're using linux here
with patch('salt.utils.platform.is_windows', lambda: False):
with patch('salt.utils.path.which', lambda exe: exe):
self.assertTrue(salt.utils.path.which('this-binary-exists-under-linux'))

def test_existing_binary_in_windows(self):
with patch('os.access') as osaccess:
with patch('os.path.isfile') as isfile:
# We define the side_effect attribute on the mocked object in order to
# specify which calls return which values. First call to os.access
# specify which calls return which values. First call to os.path.isfile
# returns X, the second Y, the third Z, etc...
osaccess.side_effect = [
# The first os.access should return False(the abspath one)
False,
# The second, iterating through $PATH, should also return False,
# still checking for Linux
isfile.side_effect = [
# The first os.path.isfile should return False due to checking the explicit path (first is_executable)
False,
# We will now also return False once so we get a .EXE back from
# the function, see PATHEXT below.
False,
# Lastly return True, this is the windows check.
True
]
# Let's patch os.environ to provide a custom PATH variable
with patch.dict(os.environ, {'PATH': os.sep + 'bin',
'PATHEXT': '.COM;.EXE;.BAT;.CMD'}):
# Let's also patch is_windows to return True
with patch('salt.utils.platform.is_windows', lambda: True):
with patch('os.path.isfile', lambda x: True):
self.assertEqual(
salt.utils.path.which('this-binary-exists-under-windows'),
os.path.join(os.sep + 'bin', 'this-binary-exists-under-windows.EXE')
)

# Patch os.access so that it always returns True
with patch('os.access', lambda path, mode: True):
# Disable os.path.islink
with patch('os.path.islink', lambda path: False):
# we're using ';' as os.pathsep in this test
with patch('os.pathsep', ';'):
# Let's patch os.environ to provide a custom PATH variable
with patch.dict(os.environ, {'PATH': os.sep + 'bin',
'PATHEXT': '.COM;.EXE;.BAT;.CMD'}):
# Let's also patch is_windows to return True
with patch('salt.utils.platform.is_windows', lambda: True):
self.assertEqual(
salt.utils.path.which('this-binary-exists-under-windows'),
os.path.join(os.sep + 'bin', 'this-binary-exists-under-windows.EXE')
)

def test_missing_binary_in_windows(self):
with patch('os.access') as osaccess:
osaccess.side_effect = [
# The first os.access should return False(the abspath one)
# The first os.access should return False due to checking the explicit path (first is_executable)
False,
# The second, iterating through $PATH, should also return False,
# still checking for Linux
# which() will add 4 extra paths to the given one, os.access will
# be called 5 times
False, False, False, False, False
]
# Let's patch os.environ to provide a custom PATH variable
with patch.dict(os.environ, {'PATH': os.sep + 'bin'}):
# Let's also patch is_widows to return True
with patch('salt.utils.platform.is_windows', lambda: True):
self.assertEqual(
# Since we're passing the .exe suffix, the last True above
# will not matter. The result will be None
salt.utils.path.which('this-binary-is-missing-in-windows.exe'),
None
)
# we're using ';' as os.pathsep in this test
with patch('os.pathsep', ';'):
# Let's patch os.environ to provide a custom PATH variable
with patch.dict(os.environ, {'PATH': os.sep + 'bin'}):
# Let's also patch is_widows to return True
with patch('salt.utils.platform.is_windows', lambda: True):
self.assertEqual(
# Since we're passing the .exe suffix, the last True above
# will not matter. The result will be None
salt.utils.path.which('this-binary-is-missing-in-windows.exe'),
None
)

def test_existing_binary_in_windows_pathext(self):
with patch('os.access') as osaccess:
with patch('os.path.isfile') as isfile:
# We define the side_effect attribute on the mocked object in order to
# specify which calls return which values. First call to os.access
# specify which calls return which values. First call to os.path.isfile
# returns X, the second Y, the third Z, etc...
osaccess.side_effect = [
# The first os.access should return False(the abspath one)
False,
# The second, iterating through $PATH, should also return False,
# still checking for Linux
isfile.side_effect = [
# The first os.path.isfile should return False due to checking the explicit path (first is_executable)
False,
# We will now also return False 3 times so we get a .CMD back from
# the function, see PATHEXT below.
# Lastly return True, this is the windows check.
False, False, False,
True
]
# Let's patch os.environ to provide a custom PATH variable
with patch.dict(os.environ, {'PATH': os.sep + 'bin',
'PATHEXT': '.COM;.EXE;.BAT;.CMD;.VBS;'
'.VBE;.JS;.JSE;.WSF;.WSH;.MSC;.PY'}):
# Let's also patch is_windows to return True
with patch('salt.utils.platform.is_windows', lambda: True):
with patch('os.path.isfile', lambda x: True):
self.assertEqual(
salt.utils.path.which('this-binary-exists-under-windows'),
os.path.join(os.sep + 'bin', 'this-binary-exists-under-windows.CMD')
)

# Patch os.access so that it always returns True
with patch('os.access', lambda path, mode: True):

# Disable os.path.islink
with patch('os.path.islink', lambda path: False):

# we're using ';' as os.pathsep in this test
with patch('os.pathsep', ';'):

# Let's patch os.environ to provide a custom PATH variable
with patch.dict(os.environ, {'PATH': os.sep + 'bin',
'PATHEXT': '.COM;.EXE;.BAT;.CMD;.VBS;'
'.VBE;.JS;.JSE;.WSF;.WSH;.MSC;.PY'}):

# Let's also patch is_windows to return True
with patch('salt.utils.platform.is_windows', lambda: True):
self.assertEqual(
salt.utils.path.which('this-binary-exists-under-windows'),
os.path.join(os.sep + 'bin', 'this-binary-exists-under-windows.CMD')
)

0 comments on commit 7b48f54

Please sign in to comment.