Some tweaks from code review: more comments; names for dev-only switches

codejail/jail_code.py

@@ -114,7 +114,9 @@ def jail_code(command, code=None, files=None, argv=None, stdin=None):
     named in the `argv` list.
 
     `files` is a list of file paths, they are all copied to the jailed
-    directory.
+    directory.  Note that no check is made here that the files don't contain
+    sensitive information.  The caller must somehow determine whether to allow
+    the code access to the files.
 
     `argv` is the command-line arguments to supply.
 
@@ -122,7 +124,7 @@ def jail_code(command, code=None, files=None, argv=None, stdin=None):
 
         .stdout: stdout of the program, a string
         .stderr: stderr of the program, a string
-        .status: return status of the process: an int, 0 for successful
+        .status: exit status of the process: an int, 0 for success
 
     """
     if not is_configured(command):
@@ -153,7 +155,8 @@ def jail_code(command, code=None, files=None, argv=None, stdin=None):
 
         subproc = subprocess.Popen(
             cmd, preexec_fn=set_process_limits, cwd=tmpdir,
-            stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+            stdin=subprocess.PIPE,
+            stdout=subprocess.PIPE, stderr=subprocess.PIPE,
         )
 
         # TODO: time limiting.  The ProcessKillerThread doesn't work yet, so
@@ -188,6 +191,9 @@ def set_process_limits():
 
 
 class ProcessKillerThread(threading.Thread):
+    """
+    A thread to kill a process after a given time limit.
+    """
     def __init__(self, subproc, limit=1):
         super(ProcessKillerThread, self).__init__()
         self.subproc = subproc
@@ -204,6 +210,8 @@ class ProcessKillerThread(threading.Thread):
         if self.subproc.poll() is None:
             # Can't use subproc.kill because we launched the subproc with sudo.
             killargs = ["sudo", "kill", "-9", str(self.subproc.pid)]
-            kill = subprocess.Popen(killargs, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            kill = subprocess.Popen(
+                killargs, stdout=subprocess.PIPE, stderr=subprocess.PIPE
+            )
             out, err = kill.communicate()
             # TODO: This doesn't actually kill the process.... :(

codejail/safe_exec.py

@@ -13,6 +13,14 @@ from codejail.util import temp_directory, change_directory
 log = logging.getLogger(__name__)
 
 
+# Flags to let developers temporarily change some behavior in this file.
+
+# Set this to True to log all the code and globals being executed.
+LOG_ALL_CODE = False
+# Set this to True to use the unsafe code, so that you can debug it.
+ALWAYS_BE_UNSAFE = False
+
+
 class SafeExecException(Exception):
     """
     Python code running in the sandbox has failed.
@@ -32,6 +40,15 @@ def safe_exec(code, globals_dict, files=None, python_path=None):
     during execution.  Modifications the code makes to `globals_dict` are
     reflected in the dictionary on return.
 
+    `files` is a list of file paths, either files or directories.  They will be
+    copied into the temp directory used for execution.  No attempt is made to
+    determine whether the file is appropriate or safe to copy.  The caller must
+    determine which files to provide to the code.
+
+    `python_path` is a list of directory paths.  They will be copied just as
+    `files` are, but will also be added to `sys.path` so that modules there can
+    be imported.
+
     Returns None.  Changes made by `code` are visible in `globals_dict`.  If
     the code raises an exception, this function will raise `SafeExecException`
     with the stderr of the sandbox process, which usually includes the original
@@ -95,7 +112,7 @@ def safe_exec(code, globals_dict, files=None, python_path=None):
     jailed_code = "".join(the_code)
 
     # Turn this on to see what's being executed.
-    if 0:
+    if LOG_ALL_CODE:
         log.debug("Jailed code: %s", jailed_code)
         log.debug("Exec: %s", code)
         log.debug("Stdin: %s", stdin)
@@ -148,6 +165,11 @@ def not_safe_exec(code, globals_dict, files=None, python_path=None):
     and modifying sys.path.
 
     """
+    # Because it would be bad if this function were used in production, let's
+    # log a warning when it is used.  Developers can can live with one more
+    # log line.
+    log.warning("Using codejail/safe_exec.py:not_safe_exec")
+
     g_dict = json_safe(globals_dict)
 
     with temp_directory(delete_when_done=True) as tmpdir:
@@ -172,7 +194,7 @@ def not_safe_exec(code, globals_dict, files=None, python_path=None):
 
     globals_dict.update(json_safe(g_dict))
 
+
 # Running Python code in the sandbox makes it difficult to debug.
-# Change 0 to 1 to run the code directly.
-if 0 or not jail_code.is_configured("python"):
+if ALWAYS_BE_UNSAFE or not jail_code.is_configured("python"):
     safe_exec = not_safe_exec