Skip to content

Fix segfault when passing invalid include path in fbtrace config via fbtracemgr - #9091

Open
Noremos wants to merge 2 commits into
FirebirdSQL:masterfrom
Noremos:fbtrace_include_fix
Open

Fix segfault when passing invalid include path in fbtrace config via fbtracemgr#9091
Noremos wants to merge 2 commits into
FirebirdSQL:masterfrom
Noremos:fbtrace_include_fix

Conversation

@Noremos

@Noremos Noremos commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Using the include directive allows you to insert configuration from a separate file. However, passing an invalid path causes the server to crash (for user sessions only). Using recursive include also leads to a crash

Steps to reproduce

  1. create user NT password '123';
  2. Create a file /tmp/trace_include_missing.conf with the following contents:
database
{
	enabled = true
}
include /a/b/c/d
  1. Execute:
    ./fbtracemgr -se <server ip/port>:service_mgr -user NT -password 123 -start -name nytrace -config /tmp/trace_include_missing.conf

After this, a segfault will occur.

Comment thread src/common/utils_proto.h

private:
T* m_value;
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a new class instead of std::optional<const char*>?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::optional does not guarantee that the contained pointer is non-null. You can still accidentally wrap a nullptr in an std::optional or forget to validate the pointer before storing it. As a result, you often end up having to perform two checks: optPointer.has_value() && optPointer.value() != nullptr (or optPointer.has_value(), fb_assert(optPointer.value() != nullptr)

This makes std::optional dangerous to use for pointer types. A SafePointer, on the other hand, guarantees that the contained pointer is never nullptr, eliminating the need for the second check.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single check: optPointer.value_or(nullptr) != nullptr.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And generally, if you check for nullptr anyway - a dumb pointer is enough. If you want nullptr to be unacceptable - a reference is used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single check: optPointer.value_or(nullptr) != nullptr.

But you can still accidentally call value() and get a null pointer. Why create unnecessary problems when they can easily be prevented with a simple wrapper?

And generally, if you check for nullptr anyway - a dumb pointer is enough. If you want nullptr to be unacceptable - a reference is used.

This code requires passing pointers to functions. But overall, I agree: using references would be a good idea.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, these functions are not engraved in stone. They can be refactored.
Second, what exactly was the problem with a dumb pointer that was used before introduction of this helper?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, these functions are not engraved in stone. They can be refactored. Second, what exactly was the problem with a dumb pointer that was used before introduction of this helper?

The problem was with null pointer dereference in the error message
(Arg::Gds(isc_conf_include) << currentFileName << parPath << Arg::Gds(isc_include_miss)).raise();

My simple fix prevents such errors once and for all. Without any hard refactoring. Just one new class and its done. Why are you getting so nervous about this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your PR fixes only this exactly case. Other possible cases where nullptr is used in construction of an error vector are untouched. If the dereference was the problem, it had to be fixed in the point where the dereference happen, i.e. in operator<<. It would cover all possible cases, not just this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exacly cases are you talking? I described the case, i fixed it. If you see other porblems with TextStream - let me known. I will include it into this fix. But if you just want to prove your point - please don't. I am tired of this pointless conversation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short: you found that some code crashes when nullptr is put into status vector as a value for isc_arg_string. That code must be fixed at first hand, this code (where nullptr is really used) is secondary.

@aafemt aafemt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the only usage of the only member of this helper is meaningless.

}

PathName tempPath(fileName);
const char* fileNameData = fileName.value_or("");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here usage of value_or is meaningless because fileName without value cannot reach this point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I decided not to include the value function to avoid potential errors.

{
PathName dummy;
PathUtils::splitLastComponent(path, dummy, currentFileName);
PathUtils::splitLastComponent(path, dummy, currentFileName.value_or(""));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is: how can you parse file without name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, whole this PR can be reduced to

        const char* getFileName() const
        {
-               return NULL;
+               return "Passed text";
        }

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants