Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap JSContext() with JSLocker() lock #42

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ianshefferman
Copy link

Make PyV8.JSContext instance thread safe by wrapping it in a lock. Multithreaded programs that call peepdf may cause peepdf analyses to mysteriously segfault without this lock. This is an issue with PyV8 in general.

Examples of other people having similar issues with PyV8 in multithreaded applications:

http://inarticulateloquence.com/django-pyv8/
http://www.pythoneye.com/440_11816513/

Those links both appear to be dead now, unfortunately, but adding this lock resolved a segfault I was observing, which I suspected was being caused by a race condition.

Copied from my earlier defunct PR at viper-framework/viper#130.

Make PyV8.JSContext thread safe by wrapping it in a lock. Multithreaded programs that call peepdf may cause peepdf analyses to mysteriously segfault without this. This is an issue with PyV8 in general.

Examples of other people having similar issues with PyV8 in multithreaded applications:

http://inarticulateloquence.com/django-pyv8/
http://www.pythoneye.com/440_11816513/

Copied from my earlier defunct PR at viper-framework/viper#130.
@jesparza
Copy link
Owner

jesparza commented Nov 1, 2015

Hi!

Thanks for the pull request ;) I have tested it and I get the following error in line 101:

File "build/bdist.linux-x86_64/egg/PyV8.py", line 176, in exit
raise RuntimeError("Lock should be released after leave the context")

The PR is also based in a previous peepdf commit, but not a big issue there...

@ianshefferman
Copy link
Author

I changed it to leave the context before releasing the lock.

I notice the context object is returned at https://github.com/ianshefferman/peepdf/blob/patch-1/JSAnalysis.py#L139, but I don't think that should affect functionality.

@jesparza
Copy link
Owner

jesparza commented Nov 8, 2015

Well, really it affects ;) This is because the context is stored globally, so when we try to check the variables existent in the context (js_vars) it fails.

Traceback (most recent call last):
File "./peepdf.py", line 710, in
console.cmdloop()
File "/usr/lib/python2.7/cmd.py", line 142, in cmdloop
stop = self.onecmd(line)
File "/usr/lib/python2.7/cmd.py", line 221, in onecmd
return func(arg)
File "/data/it/workspace/peepdf/PDFConsole.py", line 2396, in do_js_vars
varArray = context.locals.keys()
UnboundLocalError: Javascript object out of context

Isn't there a way to release the lock without leaving the context?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants