Repository: rabitt/pysox

Issue:
Base call to SoX uses shell=True in subprocess call
Filenames with wildcards have always behaved inconsistently with pysox. Looking into this more closely, found that - the SoX command arguments being passed to the shell rather than to the binary directly.

Thanks @jongwook for finding the source!

Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/18205419/badge)](https://coveralls.io/builds/18205419)

Coverage decreased (-0.009%) to 98.746% when pulling **aeb7b9f017a703caef8b3be662624c69b35c3619 on subprocess-fix** into **0c87e45ab170b6e08e5ebacc9328c6053db95710 on master**.


Patch:
diff --git a/sox/combine.py b/sox/combine.py
index 8ec8155..24854a6 100644
--- a/sox/combine.py
+++ b/sox/combine.py
@@ -11,7 +11,6 @@ from . import file_info
 from . import core
 from .log import logger
 from .core import ENCODING_VALS
-from .core import enquote_filepath
 from .core import is_number
 from .core import sox
 from .core import play
@@ -90,7 +89,7 @@ class Combiner(Transformer):
         args.extend(input_args)
 
         args.extend(self.output_format)
-        args.append(enquote_filepath(output_filepath))
+        args.append(output_filepath)
         args.extend(self.effects)
 
         status, out, err = sox(args)
@@ -305,9 +304,6 @@ class Combiner(Transformer):
         self.input_format = input_format
         return self
 
-    def splice(self):
-        raise NotImplementedError
-
 
 def _validate_file_formats(input_filepath_list, combine_type):
     '''Validate that combine method can be performed with given files.
@@ -436,7 +432,7 @@ def _build_input_args(input_filepath_list, input_format_list):
     zipped = zip(input_filepath_list, input_format_list)
     for input_file, input_fmt in zipped:
         input_args.extend(input_fmt)
-        input_args.append(enquote_filepath(input_file))
+        input_args.append(input_file)
 
     return input_args
 
diff --git a/sox/core.py b/sox/core.py
index 118cd4a..1832c81 100644
--- a/sox/core.py
+++ b/sox/core.py
@@ -14,14 +14,6 @@ ENCODING_VALS = [
 ]
 
 
-def enquote_filepath(fpath):
-    """Wrap a filepath in double-quotes to protect difficult characters.
-    """
-    if ' ' in fpath:
-        fpath = '"{}"'.format(fpath.strip("'").strip('"'))
-    return fpath
-
-
 def sox(args):
     '''Pass an argument list to SoX.
 
@@ -43,12 +35,10 @@ def sox(args):
         args[0] = "sox"
 
     try:
-        command = ' '.join(args)
-        logger.info("Executing: %s", command)
+        logger.info("Executing: %s", ' '.join(args))
 
         process_handle = subprocess.Popen(
-            command, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
-            shell=True
+            args, stdout=subprocess.PIPE, stderr=subprocess.PIPE
         )
 
         out, err = process_handle.communicate()
@@ -85,7 +75,7 @@ def _get_valid_formats():
     if NO_SOX:
         return []
 
-    so = subprocess.check_output('sox -h', shell=True)
+    so = subprocess.check_output(['sox', '-h'])
     if type(so) is not str:
         so = str(so, encoding='UTF-8')
     so = so.split('\n')
@@ -118,14 +108,14 @@ def soxi(filepath, argument):
     if argument not in SOXI_ARGS:
         raise ValueError("Invalid argument '{}' to SoXI".format(argument))
 
-    args = ['sox --i']
+    args = ['sox', '--i']
     args.append("-{}".format(argument))
-    args.append(enquote_filepath(filepath))
+    args.append(filepath)
 
     try:
         shell_output = subprocess.check_output(
-            " ".join(args),
-            shell=True, stderr=subprocess.PIPE
+            args,
+            stderr=subprocess.PIPE
         )
     except CalledProcessError as cpe:
         logger.info("SoXI error message: {}".format(cpe.output))
@@ -181,7 +171,6 @@ def play(args):
 class SoxiError(Exception):
     '''Exception to be raised when SoXI exits with non-zero status.
     '''
-
     def __init__(self, *args, **kwargs):
         Exception.__init__(self, *args, **kwargs)
 
diff --git a/sox/file_info.py b/sox/file_info.py
index 9077f1a..7f0356c 100644
--- a/sox/file_info.py
+++ b/sox/file_info.py
@@ -7,7 +7,6 @@ import os
 from .core import VALID_FORMATS
 from .core import soxi
 from .core import sox
-from .core import enquote_filepath
 
 
 def bitrate(input_filepath):
@@ -363,7 +362,7 @@ def _stat_call(filepath):
         Sox output from stderr.
     '''
     validate_input_file(filepath)
-    args = ['sox', enquote_filepath(filepath), '-n', 'stat']
+    args = ['sox', filepath, '-n', 'stat']
     _, _, stat_output = sox(args)
     return stat_output
 
diff --git a/sox/transform.py b/sox/transform.py
index df4609e..de13675 100644
--- a/sox/transform.py
+++ b/sox/transform.py
@@ -12,7 +12,6 @@ import random
 import os
 
 from .core import ENCODING_VALS
-from .core import enquote_filepath
 from .core import is_number
 from .core import play
 from .core import sox
@@ -411,11 +410,9 @@ class Transformer(object):
 
         '''
         file_info.validate_input_file(input_filepath)
-        input_filepath = enquote_filepath(input_filepath)
 
         if output_filepath is not None:
             file_info.validate_output_file(output_filepath)
-            output_filepath = enquote_filepath(output_filepath)
         else:
             output_filepath = '-n'
 
@@ -1857,7 +1854,7 @@ class Transformer(object):
             if gain[i] is not None:
                 intermed_args.append("{:f}".format(gain[i]))
 
-            effect_args.append('"{}"'.format(' '.join(intermed_args)))
+            effect_args.append(' '.join(intermed_args))
 
         self.effects.extend(effect_args)
         self.effects_log.append('mcompand')
@@ -1882,15 +1879,17 @@ class Transformer(object):
 
         '''
         if os.path.isdir(profile_path):
-            raise ValueError("profile_path {} is a directory, but filename should be specified.")
-        
+            raise ValueError(
+                "profile_path {} is a directory.".format(profile_path))
+
         if os.path.dirname(profile_path) == '' and profile_path != '':
             _abs_profile_path = os.path.join(os.getcwd(), profile_path)
         else:
             _abs_profile_path = profile_path
-                
+
         if not os.access(os.path.dirname(_abs_profile_path), os.W_OK):
-            raise IOError("profile_path {} is not writeable.".format(_abs_profile_path))
+            raise IOError(
+                "profile_path {} is not writeable.".format(_abs_profile_path))
 
         effect_args = ['noiseprof', profile_path]
         self.build(input_filepath, None, extra_args=effect_args)
@@ -1920,7 +1919,8 @@ class Transformer(object):
         '''
 
         if not os.path.exists(profile_path):
-            raise IOError("profile_path {} does not exist.".format(profile_path))
+            raise IOError(
+                "profile_path {} does not exist.".format(profile_path))
 
         if not is_number(amount) or amount < 0 or amount > 1:
             raise ValueError("amount must be a number between 0 and 1.")
@@ -2208,7 +2208,8 @@ class Transformer(object):
         >>> tfm.remix(remix_dictionary)
 
         '''
-        if not (isinstance(remix_dictionary, dict) or remix_dictionary is None):
+        if not (isinstance(remix_dictionary, dict) or
+                remix_dictionary is None):
             raise ValueError("remix_dictionary must be a dictionary or None.")
 
         if remix_dictionary is not None:
@@ -3118,7 +3119,7 @@ class Transformer(object):
         ----------
         gain : float
             Interpreted according to the given `gain_type`.
-            If `gain_type' = 'amplitude', `gain' is a (positive) amplitude ratio.
+            If `gain_type' = 'amplitude', `gain' is a positive amplitude ratio.
             If `gain_type' = 'power', `gain' is a power (voltage squared).
             If `gain_type' = 'db', `gain' is in decibels.
         gain_type : string, default='amplitude'
@@ -3160,7 +3161,7 @@ class Transformer(object):
         elif gain_type == 'db':
             effect_args.append('dB')
         else:
-            raise ValueError('gain_type must be one of amplitude, power, or db')
+            raise ValueError('gain_type must be one of amplitude power or db')
 
         if limiter_gain is not None:
             if gain_type in ['amplitude', 'power'] and gain > 1:
diff --git a/sox/version.py b/sox/version.py
index ad0d588..b718b9b 100644
--- a/sox/version.py
+++ b/sox/version.py
@@ -3,4 +3,4 @@
 """Version info"""
 
 short_version = '1.3'
-version = '1.3.5'
+version = '1.3.6'


Test patch:
diff --git a/tests/test_transform.py b/tests/test_transform.py
index 1f7795a..f611ee4 100644
--- a/tests/test_transform.py
+++ b/tests/test_transform.py
@@ -2340,18 +2340,17 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.005000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
-            '-34.000000,-17.000000,-33.000000,0.000000,0.000000"',
+            '0.005000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
+            '-34.000000,-17.000000,-33.000000,0.000000,0.000000',
             '1600.000000',
-            '"0.000625,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
-            '-15.000000,-33.000000,0.000000,0.000000"'
+            '0.000625,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
+            '-15.000000,-33.000000,0.000000,0.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
         actual_log = tfm.effects_log
         expected_log = ['mcompand']
         self.assertEqual(expected_log, actual_log)
-
         actual_res = tfm.build(INPUT_FILE, OUTPUT_FILE)
         expected_res = True
         self.assertEqual(expected_res, actual_res)
@@ -2369,8 +2368,8 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.005000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
-            '-34.000000,-17.000000,-33.000000,0.000000,0.000000"'
+            '0.005000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
+            '-34.000000,-17.000000,-33.000000,0.000000,0.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
@@ -2390,11 +2389,11 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.005000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
-            '-34.000000,-17.000000,-33.000000,0.000000,0.000000"',
+            '0.005000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
+            '-34.000000,-17.000000,-33.000000,0.000000,0.000000',
             '100.000000',
-            '"0.000625,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
-            '-15.000000,-33.000000,0.000000,0.000000"'
+            '0.000625,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
+            '-15.000000,-33.000000,0.000000,0.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
@@ -2419,11 +2418,11 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.500000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
-            '-34.000000,-17.000000,-33.000000,0.000000,0.000000"',
+            '0.500000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
+            '-34.000000,-17.000000,-33.000000,0.000000,0.000000',
             '1600.000000',
-            '"0.062500,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
-            '-15.000000,-33.000000,0.000000,0.000000"'
+            '0.062500,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
+            '-15.000000,-33.000000,0.000000,0.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
@@ -2458,11 +2457,11 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.005000,0.001000 6.000000:-47.000000,-40.000000,-34.000000,'
-            '-34.000000,-17.000000,-33.000000,0.000000,0.000000"',
+            '0.005000,0.001000 6.000000:-47.000000,-40.000000,-34.000000,'
+            '-34.000000,-17.000000,-33.000000,0.000000,0.000000',
             '1600.000000',
-            '"0.000625,0.500000 -47.000000,-40.000000,-34.000000,-34.000000,'
-            '-15.000000,-33.000000,0.000000,0.000000"'
+            '0.000625,0.500000 -47.000000,-40.000000,-34.000000,-34.000000,'
+            '-15.000000,-33.000000,0.000000,0.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
@@ -2497,11 +2496,11 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.005000,0.100000 -2.000000:-47.000000,-40.000000,-34.000000,'
-            '-34.000000,-17.000000,-33.000000,0.000000,0.000000"',
+            '0.005000,0.100000 -2.000000:-47.000000,-40.000000,-34.000000,'
+            '-34.000000,-17.000000,-33.000000,0.000000,0.000000',
             '1600.000000',
-            '"0.000625,0.012500 -5.000000:-47.000000,-40.000000,-34.000000,'
-            '-34.000000,-15.000000,-33.000000,0.000000,0.000000"'
+            '0.000625,0.012500 -5.000000:-47.000000,-40.000000,-34.000000,'
+            '-34.000000,-15.000000,-33.000000,0.000000,0.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
@@ -2516,11 +2515,11 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.005000,0.100000 -47.000000,-40.000000,-34.000000,-34.000000,'
-            '-17.000000,-33.000000,0.000000,0.000000"',
+            '0.005000,0.100000 -47.000000,-40.000000,-34.000000,-34.000000,'
+            '-17.000000,-33.000000,0.000000,0.000000',
             '1600.000000',
-            '"0.000625,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
-            '-15.000000,-33.000000,0.000000,0.000000"'
+            '0.000625,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
+            '-15.000000,-33.000000,0.000000,0.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
@@ -2555,10 +2554,10 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.005000,0.100000 6.000000:-70.000000,-60.000000,-60.000000,'
-            '-20.000000,-40.000000,-40.000000,0.000000,-4.000000"',
+            '0.005000,0.100000 6.000000:-70.000000,-60.000000,-60.000000,'
+            '-20.000000,-40.000000,-40.000000,0.000000,-4.000000',
             '1600.000000',
-            '"0.000625,0.012500 -70.000000,-60.000000,0.000000,-4.000000"'
+            '0.000625,0.012500 -70.000000,-60.000000,0.000000,-4.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
@@ -2622,11 +2621,11 @@ class TestTransformerMcompand(unittest.TestCase):
         actual_args = tfm.effects
         expected_args = [
             'mcompand',
-            '"0.005000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
-            '-34.000000,-17.000000,-33.000000,0.000000,0.000000 3.000000"',
+            '0.005000,0.100000 6.000000:-47.000000,-40.000000,-34.000000,'
+            '-34.000000,-17.000000,-33.000000,0.000000,0.000000 3.000000',
             '1600.000000',
-            '"0.000625,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
-            '-15.000000,-33.000000,0.000000,0.000000 -1.000000"'
+            '0.000625,0.012500 -47.000000,-40.000000,-34.000000,-34.000000,'
+            '-15.000000,-33.000000,0.000000,0.000000 -1.000000'
         ]
         self.assertEqual(expected_args, actual_args)
 
@@ -2650,7 +2649,7 @@ class TestTransformerMcompand(unittest.TestCase):
 
 
 class TestTransformerNoiseprof(unittest.TestCase):
-    
+
     def test_default(self):
         tfm = new_transformer()
         save_path = os.path.join(os.getcwd(), 'noise.prof')
@@ -2673,7 +2672,7 @@ class TestTransformerNoiseprof(unittest.TestCase):
         tfm = new_transformer()
         with self.assertRaises(ValueError):
             tfm.noiseprof(INPUT_FILE, os.getcwd())
-        
+
     def test_noise_prof_invalid_write(self):
         tfm = new_transformer()
         with self.assertRaises(IOError):
@@ -2689,7 +2688,7 @@ class TestTransformerNoiseprof(unittest.TestCase):
 
 
 class TestTransformerNoisered(unittest.TestCase):
-    
+
     def test_default(self):
         tfm = new_transformer()
         tfm.noisered(NOISE_PROF_FILE)
@@ -2709,7 +2708,7 @@ class TestTransformerNoisered(unittest.TestCase):
         actual_res = tfm.build(INPUT_FILE, OUTPUT_FILE)
         expected_res = True
         self.assertEqual(expected_res, actual_res)
-    
+
     def test_noise_prof_valid(self):
         tfm = new_transformer()
         tfm.noisered(NOISE_PROF_FILE)
@@ -2738,10 +2737,9 @@ class TestTransformerNoisered(unittest.TestCase):
         )
 
         actual_args = tfm.effects
-        expected_args = ['noisered',
-            NOISE_PROF_FILE,
-            '0.700000'
-            ]
+        expected_args = [
+            'noisered', NOISE_PROF_FILE, '0.700000'
+        ]
         self.assertEqual(expected_args, actual_args)
 
         actual_res = tfm.build(INPUT_FILE, OUTPUT_FILE)
@@ -4083,7 +4081,6 @@ class TestTransformerPowerSpectrum(unittest.TestCase):
         self.assertEqual(expected_last, actual[-1])
 
 
-
 class TestTransformerStats(unittest.TestCase):
 
     def test_default(self):
@@ -4131,6 +4128,7 @@ class TestTransformerStats(unittest.TestCase):
         }
         self.assertEqual(expected, actual)
 
+
 class TestTransformerSwap(unittest.TestCase):
 
     def test_default(self):
@@ -4574,7 +4572,7 @@ class TestTransformerVol(unittest.TestCase):
 
         actual_res = tfm.build(INPUT_FILE, OUTPUT_FILE)
         expected_res = True
-        self.assertEqual(expected_res, actual_res)        
+        self.assertEqual(expected_res, actual_res)
 
     def test_limiter_gain_vol_down(self):
         tfm = new_transformer()