pybind11 icon indicating copy to clipboard operation
pybind11 copied to clipboard

Add tests for accessing uninitialized holders

Open XuehaiPan opened this issue 7 months ago • 4 comments

Description

Add tests for #4549.

  • #4549

Suggested changelog entry:


XuehaiPan avatar May 12 '25 03:05 XuehaiPan

cc @rwgk @henryiii for code review

XuehaiPan avatar May 13 '25 05:05 XuehaiPan

Hi @XuehaiPan, to set expectations: Unless there is a surprise, any fix will have to come from you.

  • I recommend that you reduce this PR as much as possible ("minimal reproducer"). Concretely, reduce VectorOwns4PythonObjects to just append and size, and maybe change the name to something shorter (e.g. VecObj). Also remove all things related to the faulthandler/multiprocessing, those are just distracting you from working on a fix. (Please add [skip ci] to your commits, to not trigger the CI. The main purpose of the PR is that we can all inspect and clone it as needed.)

  • Please explain (like you would in documentation) why this is useful, and what exactly cannot be achieved with a normal init:

m.VectorOwns4PythonObjects.__new__(m.VectorOwns4PythonObjects)
  • Next, maybe, try a minimal change: preempt the apparently non-deterministic (?) undefined behavior by null-initializing the holder (?), so that you reliably get a segfault when it is dereferenced.

  • Usually, from work like that I get ideas for real fixes.

rwgk avatar May 13 '25 15:05 rwgk

BTW: You can help me following along by not force pushing.

I recommend never force pushing, just keep merging in from master (i.e. do not rebase). We'll squash-merge in the end anyway.

rwgk avatar May 13 '25 17:05 rwgk

I strongly recommend rebasing to keep following master, and never, ever create a merge commit. There's an even an automated rebase button in the dropdown below. If you create merge commits, eventually the pull request becomes unmergable and it is horrible to try to pick out the actual changes. It also becomes really hard to see what you've changed and what change upstream; eventually GitHub can't display it either. I've had a had an important long-standing PR in pypa/build die because of this. I spend hours trying to pick out the actual changes before giving up. Linear history is really important in git!

Just don't rebase to change previous commits, only to keep the PR fresh. (PS: if I'm the primary reviewer, then you can change commits too, I don't mind, I prefer the PR to tell a story. But this is the balance we've agreed to for general PRs here.)

PS: Feel free to follow @rwgk's wishes for this PR to get his help, this is general advice since he gave general advice.

henryiii avatar May 13 '25 18:05 henryiii