During exception handling check if it is not null before using

[oleksandr-pavlyk]

Description

Coverity tool flags use of "it->signature" citing that it could be a nullptr.

I suppose the thinking was that exception can only arise within the loop (so it is not null per loop termination condition), but perhaps it is better to be safe.

Suggested changelog entry:

None.

[rwgk]

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?

[oleksandr-pavlyk]

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

[oleksandr-pavlyk]

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

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.

[rwgk]

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");
            }

[oleksandr-pavlyk]

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.

[oleksandr-pavlyk]

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.

[rwgk]

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.

[oleksandr-pavlyk]

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

[oleksandr-pavlyk]

So what's next? Should I close this PR without merging since this issue is classified as a False positive?

[rwgk]

Sorry I've been very busy. I need to find a few minutes to finish up my PR with the variable name change and the assert(). Will do asap. (I can close this PR then or you can close it now.)

[rwgk]

#4817 was merged. Thanks @oleksandr-pavlyk for trying it out!