partial mock: skip forwarding of .cxx_* methods

For OCMPartialMock() objects, forwarding the .cxx_destruct method can in certain cases cause use-after-free and timing errors. The use case that triggers this behavior sits firmly as an edge case but is still easy enough to stumble into that it's worth fixing.

Conditions

To trigger this issue, partially mock a derived class. This derived class can not have any ivars that would generate .cxx_* methods. A parent of the derived class must include at least one ivar that causes .cxx_* method generation.

@interface BaseFake : NSObject {
  std::map<int, int> mapper_;
}
@end

@interface DerivedFake : BaseFake
@end

Explanation

-[OCPartialMockObject prepareObjectForInstanceMethodMocking] sets up forwarding based on -[NSObject(OCMAdditions) enumerateMethodsInClass:usingBlock:] which walks the class hierarchy to find instance methods.

Since DerivedFake has no -.cxx_destruct of its own, -[BaseFake .cxx_destruct] is found and forwarding is set up. This becomes an issue during dealloc. object_cxxDestructFromClass comes along and walks the class hierarchy again, calling all of the -.cxx_destructs that it finds. First finding one in the mock subclass of DerivedFake and calling it, which is the implementation from BaseFake, so BaseFake’s .cxx_* managed ivars are torn down early, before DerivedFake is completely torn down, which is itself a timing problem. Then, a second time, it finds -[BaseFake .cxx_destruct] when looking at BaseFake in the hierarchy. It's the same implementation, it calls it again, and this is when use-after-free can take place.

This Pull

This pull adds .cxx_construct and .cxx_destruct to the list of methods not to be forwarded. By not setting up forwarding for .cxx_destruct we can avoid the multiple calls to .cxx_destruct.

It also adds a regression test.

Note .cxx_construct will never be called for a partially mocked object. It is present in the methodsNotToForward array for technical completeness.

Notes

I am very open to feedback and would be happy to discuss any alternative approaches.
The testPartialMockBaseCXXDestruct test is expected to pass regardless of .cxx_* method forwarding. It's mainly here to test any future unknown regressions.
testPartialMockDerivedCXXDestruct is the regression test for this pull and is expected to fail when.cxx_* methods forwarding is in place.
It is my understanding that OCMClassMock() is unaffected by this issue

Project Questions

Do you prefer the tests to be in both OCMCPlusPlus98Tests and OCMCPlusPlus11Tests, or just one of them? Given the current content of the two files I duplicated the tests in each. If this is desired do you have any suggestions for sharing code between them? Is it okay to create some helper files that are used by both tests? If so, where do you prefer they live?

Thanks

Thanks to @markmentovai for debugging this with me and for a lot of the explanation copy!
Thanks @mlw for discovering this bug and for debugging this with me!

erikdoe / ocmock