scottp-dpaw commented 4 years ago

Callbacks set by Persistent::SetWeak aren't guaranteed to be called by V8, even if the context gets destroyed. As such, the majority of Python objects passed into the context will never be freed.

To fix this, this patch replaces js_object_cache (which was a weakref.WeakKeyDictionary) with a normal dict. This ensures the reference count is managed with the lifetime of the dictionary.

codecov-commenter commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@4fe3eea). Click here to learn what that means. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master      #38   +/-   ##
=========================================
  Coverage          ?   80.22%           
=========================================
  Files             ?       16           
  Lines             ?     1345           
  Branches          ?        0           
=========================================
  Hits              ?     1079           
  Misses            ?      266           
  Partials          ?        0

Impacted Files	Coverage Δ
v8py/context.cpp	`86.99% <100.00%> (ø)`
v8py/pyclass.cpp	`96.12% <100.00%> (ø)`
v8py/kappa.cpp	`100.00% <0.00%> (ø)`
v8py/pyfunction.cpp	`100.00% <0.00%> (ø)`
v8py/v8py.cpp	`86.66% <0.00%> (ø)`
v8py/pyclasshandlers.cpp	`75.34% <0.00%> (ø)`
v8py/kappa.h	`100.00% <0.00%> (ø)`
v8py/debugger.cpp	`10.00% <0.00%> (ø)`
v8py/convert.cpp	`84.90% <0.00%> (ø)`
v8py/polyfill.h	`83.33% <0.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4fe3eea...72018c6. Read the comment docs.

tbodt commented 4 years ago

This will no longer free the objects if you have a single long-lived context and the weak callback does end up getting called. Can you fix that?

scottp-dpaw commented 4 years ago

From our experiences in production, SetWeak barely ever gets called (even with the context deleted and the v8 garbage collector pumped), and the v8 docs state SetWeak shouldn't be used for freeing resources because of this. That aside, could you give an example of the circumstances that the weak callback would be called, outside of deleting the context? My very rough understanding on how this works:

Python object tree gets passed to a v8py.Context
v8py iterates through this, creates the v8 primitive equivalent for each Python object, and adds it to js_object_cache
JS scripts are run within the context, values from the JS objects are written back to the Python objects
Context gets cleaned up, js_object_cache gets deleted, existing references mean the Python objects live

I'm guessing, but would the only other time SetWeak gets invoked be if v8 frees a JS object for whatever reason, after which the entry should be removed from js_object_cache?

tbodt commented 4 years ago

It sounds like in your use case the context gets cleaned up pretty quickly, but I can imagine other use cases where you create a single context that lives for a long time (e.g. a theoretical Python rewrite of Node). If you do that with this PR the js_object_cache would grow without bound. The only hope for solving that problem is hoping V8 will call the weak callback eventually.

Also this introduces a UAF: if you add a Python object to context A, make that Python object accessible from context B without going through py_class_create_js_object (a.foo = {'x': object()}; b.foo = a.foo), and then destroy context A, the Python object will be freed while it can still be accessed from JavaScript.

I just had a look at how Chromium deals with this. The code is at https://source.chromium.org/chromium/chromium/src/+/master:gin/wrappable.cc. Ctrl-F "delete" shows that they delete the object from the weak callback, and also if NewInstance() returns nothing (which in v8py will just crash, so apparently this never happens?). So the V8 docs say not to rely on the weak callback for freeing resources, while Chromium does anyway. Not sure who to believe.

scottp-dpaw commented 4 years ago

The use-after-free I can't seem to replicate; my understanding is the refcount for a Python object is increased when it is added to each context, so removing one context altogether wouldn't be enough to deallocate the object.

After maybe the most punishing troubleshooting process ever, I think I have a replacement weak callback. As part of garbage collection v8 will trash the contents of the v8::Context before the weak callback gets invoked, so it's impossible to use context->GetEmbedderData. I had to add another pointer for js_object_cache to each js object.

scottp-dpaw commented 4 years ago

Everything should be sorted now, is there anything else required for review? It would be fantastic if this could be rolled into a point release, we've found with this fix in place v8py-using threads use roughly half the memory.

tbodt commented 9 months ago

I've decided to accept the reality that I don't have time or motivation to maintain this project anymore and add a section in the readme asking for maintainers. This PR seems like evidence that you care about the project more than me :) so please reach out if you're interested in maintaining.

tbodt / v8py

Fix Python reference count leak #38

Codecov Report