Firebird crashes if a plugin factory returns nullptr and no error in status

[aafemt]

If a trace plugin's factory returns nullptr and no error in status - Firebird crashes trying to reference this pointer. Returned value should be checked for nullptr in any case.

[hvlad]

What is Firebird version and call stack of crash ?

[aafemt]

Firebird 4.0.4.2981

No stack trace but I guess the crash occur on TraceManager.cpp:148 line info.factory->addRef(); after previous line assigned nullptr returned from traceItr.plugin() to info.factory because GetPlugins::getPlugin() check status for error but doesn't check returned value.

And this code is the same in all versions.

[hvlad]

It is still not clear what do you speak about - the bug in TraceManager ? in PluginManager ? in some implementation of custom (trace) plugin ? It is important to know to fix a root issue, not particular case (what case?)

[aafemt]

IMHO it is a bug in TraceManager. It must not trust plugin's sanity and check returned values.

[hvlad]

Did you check another callers of GetPlugins::plugin() ?

[aafemt]

I'm sorry, my guess was wrong. The returned value is checked there by hasData() call.

But in ConfiguredPlugin::factory() value returned by createPlugin() isn't checked before calling setOwner() unless I missed something again.

[AlexPeshkoff]

On 5/5/24 01:28, Dimitry Sibiryakov wrote:

I'm sorry, my guess was wrong. The returned value is checked there by |hasData()| call.

But in |ConfiguredPlugin::factory()| value returned by |createPlugin()| isn't checked before calling |setOwner()| unless I missed something again.

We anyway have to trust code in plugins cause it runs in the same process as native FB code. And if plugin factory returned no error it must return valid interface pointer. In this case plugin should be fixed. It can cause segfault in 100500 other ways, what a sense to fux one case?

[hvlad]

I'm sorry, my guess was wrong.

Don't you think that issue title should be changed ?

[aafemt]

And if plugin factory returned no error it must return valid interface pointer. In this case plugin should be fixed. It can cause segfault in 100500 other ways, what a sense to fux one case?

100500 other places already protected by hasData() call as I said above. nullptr is a valid pointer. At least specs doesn't say otherwise. Factory may have a reason to refuse return plugin object not related to any error and just want the engine to proceed to the next plugin in list.

[AlexPeshkoff]

For me valid pointer is one that may be de-referenced successfully. But I do not want to waste time with this discussion any more - in this place additional check does not cause something bad, add it please.

[aafemt]

in this place additional check does not cause something bad, add it please.

Do you mean an additional check besides one that is added in the PR?