Skip to content

Guard against null pyLocation comparisons - #313

Open
dpogue wants to merge 1 commit into
H-uru:masterfrom
dpogue:loc-compare
Open

Guard against null pyLocation comparisons#313
dpogue wants to merge 1 commit into
H-uru:masterfrom
dpogue:loc-compare

Conversation

@dpogue

@dpogue dpogue commented Jul 26, 2026

Copy link
Copy Markdown
Member

I was getting segfaults in Blender when importing with ZLZ because of location comparisons where one side was None. There's probably something worth digging into on the ZLZ side about how that happened, but we should also guard against that (reasonably valid) case and not crash in libHSPlasma.

This matches the pattern in pyKey that checked for a Py_None object and handled that separately.

I was getting segfaults in Blender when importing with ZLZ because of
location comparisons where one side was None. There's probably something
worth digging into on the ZLZ side about how that happened, but we
should also guard against that (reasonably valid) case and not crash in
libHSPlasma.
@dpogue

dpogue commented Jul 26, 2026

Copy link
Copy Markdown
Member Author

oooh, and this won't even conflict with the work in 302 because that one didn't touch the Python side 😁

@dgelessus dgelessus 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.

Hmm, it looks like the PY_PLASMA_RICHCOMPARE_DECL macro in general is incorrect here. The macro declares both left and right as having the class that's being implemented, but according to the tp_richcompare docs, that's only guaranteed for the first parameter. If the macro correctly declared right as PyObject*, this problem would become obvious at compile time.

The tp_richcompare docs also say that for unsupported comparisons, the function should return Py_NotImplemented rather than setting an error. I wonder, if we just return Py_NotImplemented if right doesn't have the expected class, would Python take care of the rest? It doesn't seem right to add a special case just for None. (I understand that you're matching what pyKey is already doing, but it would be nice to use the opportunity to implement this properly.)

@Hoikas

Hoikas commented Jul 26, 2026

Copy link
Copy Markdown
Member

We should verify that Python definitely does the right thing in that situation. Doing if location != None: is not Pythonic, but it would not be acceptable for that to raise an exception IMO.

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