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

During exception handling check if it is not null before using #4814

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1168,7 +1168,7 @@ class cpp_function : public function {
if (!result) {
std::string msg = "Unable to convert function return value to a "
"Python type! The signature was\n\t";
msg += it->signature;
msg += (it) ? it->signature : std::string("unavailable");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm worried about this change:

  • It's a behavior change (segfault vs substituting a ~random value).
  • This could make it much more difficult to find bugs introduced in modernizations.

Human inspection of the code here is made very difficult because a 2-character variable name (it) that is also a common english word is used over the scope of 451 lines. I decided that's the first thing we need to fix:

#4817

With that it's much easier to see that Coverty might have a point, therefore I also added assert(curr_overl != nullptr); in the only place where that condition could potentially false (unless I'm missing something, plmk).

Could you please run Coverty for PR #4817? To make this easy, you could simply insert assert(it != nullptr); in this PR and undo your change here. Is Coverty happy with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me try and report back, hoping for early next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reran the Coverity scan for my project using pybind11 source from #4817, the issue is still there

image

If you feel like this issue is a false positive, please feel free to close this PR.

I would keep changes in #4817 though. Does make code easier to read.

Copy link
Collaborator

@rwgk rwgk Aug 28, 2023

Choose a reason for hiding this comment

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

Thanks for trying.
Yes I believe it's a false positive, but I'd like to try a little more if we can silence it in a reasonable way, with a reasonably small effort.
Two ideas/questions:

  • Does Coverty actually consider the added assert()? I guess there could be a rationale for ignoring it, not sure.
  • Could you please add the assert() also right before Coverty thinks there is a problem (your latest screenshot)? I.e.:
            assert(curr_overl != nullptr);
            msg += curr_overl->signature;

Does that help?

If not, could you please try this instead of that assert()?

            if (curr_overl == nullptr) {
                pybind11_fail("Internal Error: Unexpected: curr_overl == nullptr");
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coverity ignores assert because I am building my project in non-debug mode. Let me try adding the suggested error check with raising of internal error. It should work.

Copy link
Contributor Author

@oleksandr-pavlyk oleksandr-pavlyk Aug 29, 2023

Choose a reason for hiding this comment

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

I can confirm that applying the following change:

(dev_dpctl) [20:18:36 ansatnuc06 pybind11]$ git diff HEAD~1..HEAD
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index c998a4a8..b25de4fc 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -1169,7 +1169,11 @@ protected:
         if (!result) {
             std::string msg = "Unable to convert function return value to a "
                               "Python type! The signature was\n\t";
-            msg += curr_overl->signature;
+            if (curr_overl == nullptr) {
+                pybind11_fail("Internal Error: Unexpected: curr_overl == nullptr");
+            } else {
+                msg += curr_overl->signature;
+            }
             append_note_if_missing_header_is_suspected(msg);
             // Attach additional error info to the exception if supported
             if (PyErr_Occurred()) {

to code in #4817 resolves the Coverity issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's great to know, thanks!
Is it possible to run Coverty without NDEBUG defined? That would be best IMO. Then we could use the concise assert() and it wouldn't impact production builds at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, running Coverity analysis on release build with -O3 -UNDEBUG the issue also disappears.

append_note_if_missing_header_is_suspected(msg);
// Attach additional error info to the exception if supported
if (PyErr_Occurred()) {
Expand Down
Loading