Reloading a plugin with test cases leads to a segmentation fault

[Schneegans]

Description

Reloading a plugin (e.g. a shared library loaded at runtime) which registers test cases, leads to a segmentation fault. It seems that registering the same tests twice causes problems... a workaround is below!

Steps to reproduce

This is a bit difficult, with my compiler this example, if modified so that it loads the plugin twice does not crash, even if I think it actually should.

Anyway, the debugger shows that the operator< of the TestCase causes the segmentation fault when inserting the test cases of the plugin a second time in the static std::set<TestCase> data; below (around line 1070):

// all the registered tests
std::set<TestCase>& getRegisteredTests() {
    static std::set<TestCase> data;
    return data;
}

This seems quite logical as the library from which the existing TestCase was allocated has been unloaded and comparing the new TestCase with the old one should therefore lead to a segmentation fault.

Workaround

Instead of a std::set, I used a std::map:

std::map<String, TestCase>& getRegisteredTests() {
    static std::map<String, TestCase> data;
    return data;
}

and I insert the tests like this:

// used by the macros for registering tests
int regTest(const TestCase& tc) {
    getRegisteredTests().emplace(toString(tc.m_line) + String(tc.m_file) + toString(tc.m_template_id), tc);
    return 0;
}

and voilà, everything works as I expect. What do you think? Is there a better way to fix this? Actually unregistering tests when unloading a shared library would be cleaner...

Extra information

• doctest version: current master
• Operating System: Ubuntu 19.04
• Compiler+version: gcc 8.3.0

[onqtam]

Thanks for reporting this! I'm not sure how I haven't discovered this - I think I had tested reloading plugins which registered test cases... Anyway.

Can you try a different fix locally - can you change the m_file member of TestCaseData to be a doctest::String instead of a const char* (also switch from std::strcmp to String::compare in TestCase::operator<) so that it just works with a std::set as it is right now? I think that currently m_file points to a location of the unloaded older version of the dll but switching to a doctest::String would force a heap-allocated copy of that string.

I also think that with both your and my fix the program would crash when running the tests after a reload - because the m_test member of the TestCase struct (which is basically a function pointer) would point to the address of the old dll too... But why would anyone run tests after a reload anyway?!

I'll think about this some more tomorrow - ideally tests should get unregistered when a .dll unloads but I'll have to think about that - I'm not sure that destructors get called properly for globals on all platforms and stuff like that...

[Schneegans]

I can confirm that this fixes it as well! And for the fun of it, I tried running tests from the reloaded plugin. As you anticipated - it crashes. However this can be fixed by overwriting the TestCase items in the set.

// used by the macros for registering tests
int regTest(const TestCase& tc) {
    auto it = getRegisteredTests().find(tc);
    if (it != getRegisteredTests().end()) {
        getRegisteredTests().erase(it);
    }
    getRegisteredTests().insert(tc);
    return 0;
}

But you are right: I don't see the use case either and this definitely comes at a slight performance cost.

[onqtam]

I pushed the fix I suggested in the dev branch. However I didn't add the check when reloading a test case to see if it's already registered so that it gets updated because it won't solve all problems:

• if the reloaded plugin has the same test case but on a different line because of a code change - we would still have the old version from the old position registered with a stale function pointer ==> crash when running it
• if a plugin is just unloaded and not reloaded we would still have broken pointers

The solution would be to actually unregister test cases when shared objects are unloaded, but I think that is too niche of a problem and thus I won't be solving it - unless someone asks for it of course :)

[Schneegans]

Great! Thank you very much for the quick response & fix!