pybind11_getbuffer: Fix Potential Nullptr Dereference

[ax3l]

Fix a potential nullptr dereference of obj.

Found with coverity in a downstream project.

[bstaletic]

@ax3l Only your team can actually see your dashboard, are we 100% sure this isn't a false positive? I know vim's codebase has some false positives.

[wjakob]

In what circumstance would obj be nullptr? Generally not a fan of adding these types of checks everywhere just to make Coverity happy. (pybind11 is a header file library, so this will have an impact on the size of bindings in all projects using it).

[zhihaoy]

Later in the code there is a check to tell whether view is nullptr, so I guess it's may be legitimate to check obj as well.

[ax3l]

Since even obj itself is checked below it should also be checked before first accessing it at Py_TYPE(obj)->tp_mro, no?

[ax3l]

@bstaletic sure, if you like I can give you access (gitter me, I think via mail). I just did not filter my "thirdParty" dir, that's why they show up and they look useful, regarding public struct members.
Of course, the question stands as @wjakob states correctly for structs that are only used with "init-wrapper" c-style functions, but I think most of the constructor init does not hurt.

[bstaletic]

@ax3l is right. Currently, on line 466 obj is dereferenced, but is checked against nullptr only later on line 471. Since NULL dereference isn't allowed in valid C or C++, compiler is allowed to remove all null checks after dereferencing, assuming that they aren't true. Therefore, from a formally correct C++ stand point, the line 471 might as well have obj == nullptr removed. Either pybind needs to check if obj is NULL earlier, or assume it never will be NULL. Either way the obj == nullptr on line 471 is useless.

[ax3l]

Alternative implementation following @wjakob's comment about obj being safe by contract in #1664 (also @bstaletic 's suggestion).

Generally not a fan of adding these types of checks everywhere just to make Coverity happy.

Coverity only complains that we do a nullptr check after already dereferencing it, because it thinks we do so for a good reason and just too late. It does not enforce checking all pointers passed to a free standing function for null, as that might be ensured by contracts already.

[wjakob]

Closing this as #1664 was merged.