Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overload Signature: Potential Nullptr Deref #1659

Closed
wants to merge 1 commit into from

Conversation

ax3l
Copy link
Collaborator

@ax3l ax3l commented Jan 7, 2019

Fix a potential nullptr in error handling. The it variable is already checked in the try block for non-nullptr and might therefore be nullptr in this branch as well.

Found with coverity in a downstream project.

@@ -763,7 +763,10 @@ class cpp_function : public function {
} else if (!result) {
std::string msg = "Unable to convert function return value to a "
"Python type! The signature was\n\t";
msg += it->signature;
if (it == nullptr)
msg += "no signature found (nullptr)";
Copy link

Choose a reason for hiding this comment

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

Please change msg to say so; the full phrase

The signature was
no signature found

reads weird.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makes sense, updated!

Fix a potential nullptr in error handling.
The `it` variable is already checked in the `try`
block for non-nullptr and might therefore be `nullptr`
in this branch as well.

Found with coverity in a downstream project.
@wjakob
Copy link
Member

wjakob commented Jun 10, 2019

The catch block can never be reached while it is nullptr. This is a case where coverity lacks context and is too conservative.

@wjakob wjakob closed this Jun 10, 2019
@ax3l
Copy link
Collaborator Author

ax3l commented Jun 10, 2019

Actually coverity took the following logic: in line 460 with for (; it != nullptr; it = it->next) { we check that it is not a nullptr.

Coverity consequently assumes it can be a nullptr and warns at the line marked in this PR.

@wjakob
Copy link
Member

wjakob commented Jun 21, 2019

During function dispatch, there are 3 basic outcomes:

  1. No matching overload can be found
  2. An exception is thrown
  3. A matching overload is found and executes, but we can't convert the return value.

The case you wanted to patch is related to #3, and it can only happen if #1 and #2 were not taken (and in this case, it != nullptr).

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