bpo-37140 ctypes change made clang fail to build

[paulmon]

Passing structs on Linux follows a different convention than Windows.
Add ifdefs to keep both conventions.

@zooba, @zware

https://bugs.python.org/issue37140

[paulmon]

If I revert the Windows code to match the previous code then test_pass_by_value (ctypes.test.test_structures.StructureTestCase) fails, hence the ifdefs.

It seems likely that there is a still a bug on Windows if self->b_ptr contains a ctypes Structure with a destructor that frees resources because the Windows code uses memcpy to do a copy of the struct data without refcounting. This can leave a dangling pointer as in the clang binding example which throws an exception on the second free.

see
memcpy(new_ptr,` self->b_ptr, self->b_size);

[vstinner]

It would be great to be able to write a reproducer and then convert it into a proper unit test.

[paulmon]

It seems like it's not possible to have pass-by-value make a copy and also not duplicate pointers in structs. When the struct has a destructor as in this unittest the copy causes the pointer to be freed twice which causes an access violation.

Is it wrong to make a copy?

Is there a way to modify the struct implementation so that it doesn't cause an exception to be thrown when passed by value?

[paulmon]

I did some reading about parameter passing and it's still not clear to me whether https://bugs.python.org/issue37140 is a bug in CPython or whether the clang bindings were relying on incorrect parameter passing behavior to work.

The change in the PR restores the previous behavior where Windows and non-Windows builds pass structs differently.

[zooba]

I think this needs to be PyTuple_Pack(2, self, copied_self) for the case where we have both, along with correct refcounting. The rest of the change to this file should be safe to undo.

[paulmon]

@zooba, are you saying that the ref counting in the test code needs fixed (and the clang bindings code)?

[paulmon]

@zooba @encukou
This passes the test now.
Does this look like the right thing to do?

[vstinner]

I cannot explain why, but this change creates a huge memory leak. It seems like copied_self is no longer destroyed. I guess that it's reference counter no longer reach zero, which works around the bug. This change doesn't fix the root issue: if copied_self would be destroyed, the structure finalizer (del) would still be caled twice.

I wrote a different fix PR #15612 which fix https://bugs.python.org/issue37140 regression and does not introduce a memory leak.

[vstinner]

This PR leaks memory. I suggest to use my fix PR #15612 instead.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[vstinner]

I merged my PR #15612, but thanks for this PR: it helped me to understand the root issue!