Securing git execution. #348

deathaxe · 2016-12-05T18:50:17Z

The documentation about python's subprocess module makes some recommendations
about how to handle subprocess calls, which are applied by this commit.

Python says:

1. communicate()

Use communicate() rather than .stdin.write, .stdout.read or .stderr.read to
avoid deadlocks due to any of the other OS pipe buffers filling up and blocking
the child process.

So maybe we should use it.

2. handle timeouts

The child process is not killed if the timeout expires, so in order to cleanup
properly a well-behaved application should kill the child process and finish
communication.

Would be nice but is not supported by python 2.6 and therefore does not work
with SublimeText 2.

3. handle OSError

The most common exception raised is OSError. This occurs, for example, when
trying to execute a non-existent file. Applications should prepare for OSError
exceptions.

Catch any exception, print its text to console and handle as empty output. Finally
resolve the Promise to avoid memory leaks due to unresolved zombie promises.

The documentation about python's `subprocess` module makes some recommendations about how to handle subprocess calls, which are applied by this commit. ### Python says: #### 1. communicate() > Use communicate() rather than .stdin.write, .stdout.read or .stderr.read to avoid deadlocks due to any of the other OS pipe buffers filling up and blocking the child process. So maybe we should use it. #### 2. handle timeouts >The child process is not killed if the timeout expires, so in order to cleanup properly a well-behaved application should kill the child process and finish communication. Would be nice but is not supported by python 2.6 and therefore does not work with SublimeText 2. #### 3. handle OSError > The most common exception raised is OSError. This occurs, for example, when trying to execute a non-existent file. Applications should prepare for OSError exceptions. Catch any exception, print its text to console and handle as empty output. Finally resolve the Promise to avoid memory leaks due to unresolved zombie promises.

rchl · 2016-12-05T19:21:26Z

git_gutter_handler.py

 import re
 import codecs
 import tempfile
 import time
 from functools import partial
+from subprocess import (
+    Popen, PIPE, STARTUPINFO, STARTF_USESHOWWINDOW)


Importing STARTUPINFO and probably the one after will throw on non-Windows.

(IMO better to just import whole subprocess like before.)

I saw GitSavvy to import only those parts of libraries in some cases, they really needed and thought this might be a good idea in general such as you do with all the objects from git_gutter_compare module. And I expected python to throw an exception anyway, if it would not find STARTUPINFO, but this is wrong. Didn't know python ignores code blocks in if clauses at all, if the condition is not true.

Will revert this.

rchl · 2016-12-05T19:27:41Z

git_gutter_handler.py

+                    startupinfo=startupinfo)
+                stdout, stderr = proc.communicate()
+            except Exception as exception:
+                print("GitGutter failed to run git: " + str(exception))


I think it's generally preferred to use string formatting in python:

print("... %s" % exception).

Hey Python, whatever happen to

There should be one-- and preferably only one --obvious way to do it.

😂

Are there any drawbacks using formattings? I am quite used to use the + for string concating in other languages, in some it is the only possible one ;-). PythonSpeed even recommends "".join() for concation.

I am totally with you formatting is the cleanest way with more complex string formatting.

We should just decide for one solution, that's rigt. As well as with quotation I think. Most strings have single quotes in the code and I accidently used double quotes here :-/

You should use "".join() when you build a sequence of many strings not to concat only 2 strings. First build them with an array and at the end join the array.

I personally prefer "{0}".format(x) over %.
However in such cases I would just use print("GitGutter failed to run git:", exception) and add from __future__ import print_function to the top for ST2 compat.

And which of all that solutions is agreed by everybody? I ask kindly to finish the discussion on such details quickly as the current small PR's were meant to be the base for two bigger ones, which intend to fix some real open issues and implement some requested features.

The % is used in other modules, too. But your last suggestion does not look so bad, too ;-) as I also try to avoid format strings where possible in other projects.

rchl · 2016-12-05T19:31:44Z

git_gutter_handler.py

+                    args=args, stdin=PIPE, stdout=PIPE, stderr=PIPE,
+                    startupinfo=startupinfo)
+                stdout, stderr = proc.communicate()
+            except Exception as exception:


Catching "any" exception is generally not recommended.
Why not just catch OSError?

To be fair why catch any exception at all if you are not doing any special handling but just re-printing it?

The main idea was to make sure the promise is resolved which is ensured by finally now. I started to catch OSError and TimeoutError (which is not supported by python 2.6) as descriped in pythons docs. But when I renamed my git.exe to check what happens if it is not found, I saw several other Exceptions in console because of args[0] == None. I found the traceback string not very useful here and it always looks like program error, but some of them might be quite usual, so just printing the exception's text only might be enough. This is what GitSavvy does on exceptions with starting git. Of course you are right, OSError should be the only one being thrown here at all, but due to some issues in the settings module, the situation of missing git.exe is not handled and settings.git_binary_path is None which causes Exceptions here. But I think we should handle this in a seperate topic as there are some more things to check with settings.

The alternitive might be to put some args checks to avoid Exceptions and catch OSError only, but I thought just catching all is easier and enough.

- Importing only required modules like STARTUPINFO fails on Linux. - Use string formatting

deathaxe · 2016-12-06T20:50:26Z

A TODO I learned from this for future is to handle git errors itself.

rchl · 2016-12-08T09:18:57Z

handle timeouts
Would be nice but is not supported by python 2.6

You could still handle it in ST3 bu using conditional block based on a constant defined earlier. Like in git_gutter_events.py, for example:

ST3 = int(sublime.version()) >= 3000

...

if ST3:
  process.communicate(timeout=15)
else
  process.communicate()

…Exceptions. If python 3 interpreter is recognized in the import block, the timeout handling of `Popen` is enabled. (Thanks rchl). Otherwise a dummy TimeoutExpired class is created for python 2.6 OS errors are caught as documentation wants, but anything else must be prevented with other solutions. To avoid ValueErrors and IndexErrors due to invalid `args` elements, the `git_gutter` command is disabled at all, if no `git_binary_path` was found by settings module.

rchl · 2016-12-08T20:48:28Z

git_gutter_handler.py

@@ -12,11 +12,20 @@
    from . import git_helper
    from .git_gutter_settings import settings
    from .promise import Promise
+    from subprocess import TimeoutExpired


Please have it in a separate try/except block.
You don't want to try to import everything from except block if this fails and you don't want to assume that TimeoutExpired doesn't exist if some other import failed to be imported.

This try ... except block is to check, if python 3 is used with fallback to 2 only, isn't it? If any of those imports fails it's most likely due to python 2. If it was due to missing object, it would fail in except block, too, which would cause the whole module not to work, right?

rchl · 2016-12-08T20:49:28Z

git_gutter_handler.py

 except (ImportError, ValueError):
    import git_helper
    from git_gutter_settings import settings
    from promise import Promise

+    class TimeoutExpired():


Maybe makes sense to inherit from "Exception". Even if it won't really be triggered.

I expected it to catch anything then, just as I would write except Exception.

rchl · 2016-12-08T20:56:37Z

git_gutter.py

@@ -26,7 +28,7 @@ def __init__(self, *args, **kwargs):
        self.show_diff_handler = None

    def is_enabled(self, **kwargs):
-        return self.is_valid_view
+        return self.is_valid_view and bool(settings.git_binary_path)


That might be pre-existing flaw, but if you are just accessing a 'git_binary_path' property of settings like this, you don't have guarantee that it was initialized (that load_settings() was called). Maybe you should add __get__ descriptor on the Settings class that will ensure that settings are loaded before returning the value.

git_binary_path is accessed this way in git_handler, too. Why is it wrong to do it exactly the same way here? And what if load_settings() was not yet run? Nothing. The command is handled as disabled by SublimeText until settings are loaded and git_binary_path is None. I don't see any issues with accessing it this way at all.

Btw.: ;-) Settings module needs some review anyway, but pleeaase give me a chance to commit the 'Enhancement for compare against' and 'Fix for the gutter icons flickering' first.

It's just not correct. It probably only works by chance because GitGutterEvents trigger settings load when using settings.get('foo'). But it's not a new problem so can keep it for now.

Just to ensure I don't miss something:

settings.load_settings() is called by git_gutter_settings.plugin_loaded() which itself is called by ST3 as soon as API is ready and on loading the module by ST2. No API call like sublime.load_settings() must be called before this point anyway. So even a getter which tries to load settings would most likely fail at this point anyway. This change activly disables git_gutter command if API is not yet ready and therefore no settings are available anyway and thus prevents running git_handler with an empty args[0], which would cause an exception. SublimeText can fire events, but GitGutter ignores them until ready. And if no git was found at all, GitGutter shows its message box triggered in settings.load_settings() and is disabled then.

So seriously: There are very obvious issues with settings and how it stores values and reloads them from the backend files, but I can't see this to be one of them.

In the future I could imagine to try to update git_binary_path if git command fails with error 2 to check activly for changed location, but this is not what this PR intends to add.

This PR intents to prevent exceptions from resolving the Promise, which is created to run the git process. Issues with the settings module should go to a seperate one, I think.

rchl · 2016-12-09T16:39:25Z

git_gutter_handler.py

@@ -12,11 +12,22 @@
    from . import git_helper
    from .git_gutter_settings import settings
    from .promise import Promise
+


unnecessary addition

rchl · 2016-12-09T16:39:46Z

git_gutter_handler.py

+try:
+    from subprocess import TimeoutExpired
+    _HAVE_TIMEOUT = True
+


wouldn't have a blank line here

rchl reviewed Dec 5, 2016

View reviewed changes

Review changes:

079bf92

- Importing only required modules like STARTUPINFO fails on Linux. - Use string formatting

rchl reviewed Dec 8, 2016

View reviewed changes

Handle TimeoutExpired import in seperate try..catch block.

5357eca

rchl reviewed Dec 9, 2016

View reviewed changes

git_gutter_handler.py

try:

from subprocess import TimeoutExpired

_HAVE_TIMEOUT = True

Copy link

Collaborator

rchl Dec 9, 2016

Choose a reason for hiding this comment

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

wouldn't have a blank line here

Coding Style: Remove unnecessary blank lines.

1ddb911

rchl approved these changes Dec 9, 2016

View reviewed changes

deathaxe merged commit 6db9f2c into jisaacks:master Dec 9, 2016

deathaxe deleted the pr/secure-git branch December 9, 2016 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Securing git execution. #348

Securing git execution. #348

deathaxe commented Dec 5, 2016

rchl Dec 5, 2016 •

edited

Loading

deathaxe Dec 6, 2016

rchl Dec 5, 2016

jisaacks Dec 5, 2016

deathaxe Dec 6, 2016

r-stein Dec 7, 2016

deathaxe Dec 7, 2016

rchl Dec 5, 2016

rchl Dec 5, 2016

deathaxe Dec 6, 2016

deathaxe commented Dec 6, 2016

rchl commented Dec 8, 2016

rchl Dec 8, 2016

deathaxe Dec 8, 2016

rchl Dec 8, 2016

deathaxe Dec 8, 2016

rchl Dec 8, 2016 •

edited

Loading

deathaxe Dec 8, 2016

rchl Dec 9, 2016

deathaxe Dec 9, 2016

rchl Dec 9, 2016

rchl Dec 9, 2016

Securing git execution. #348

Securing git execution. #348

Conversation

deathaxe commented Dec 5, 2016

Python says:

1. communicate()

2. handle timeouts

3. handle OSError

rchl Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deathaxe commented Dec 6, 2016

rchl commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl Dec 5, 2016 •

edited

Loading

rchl Dec 8, 2016 •

edited

Loading