Skip to content

Increase ob's ref count in tp_repr to avoid accidental free#1160

Merged
lostmsu merged 1 commit intopythonnet:masterfrom
DanBarzilian:PR/fix-tp_repr-bug
Jun 17, 2020
Merged

Increase ob's ref count in tp_repr to avoid accidental free#1160
lostmsu merged 1 commit intopythonnet:masterfrom
DanBarzilian:PR/fix-tp_repr-bug

Conversation

@DanBarzilian
Copy link
Contributor

@DanBarzilian DanBarzilian commented Jun 17, 2020

What does this implement/fix? Explain your changes.

Fix incorrect dereferencing wrapper object in tp_repr function, which causes the program to crash.

Does this close any currently open issues?

...

Any other comments?

...

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • [x ] Add yourself to AUTHORS
  • [x ] Updated the CHANGELOG

@dnfadmin
Copy link

dnfadmin commented Jun 17, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@DanBarzilian DanBarzilian mentioned this pull request Jun 17, 2020
Merged
4 tasks
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2020 •
edited
Loading

Codecov Report

Merging #1160 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1160   +/-   ##
=======================================
  Coverage   86.53%   86.53%           
=======================================
  Files           1        1           
  Lines         297      297           
=======================================
  Hits          257      257           
  Misses         40       40
Flag Coverage Δ
#setup_linux 64.98% <ø> (ø)
#setup_windows 72.05% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fb545a...89d1177. Read the comment docs.

@filmor filmor requested a review from lostmsu June 17, 2020 09:25
@filmor
Copy link
Member

filmor commented Jun 17, 2020

@lostmsu We should probably do a point-release for this, agree?

@lostmsu
Copy link
Member

lostmsu commented Jun 17, 2020

@filmor I have another one too in the recent params array change.

@lostmsu lostmsu mentioned this pull request Jun 17, 2020
Closed
@lostmsu lostmsu merged commit 742463e into pythonnet:master Jun 17, 2020
@lostmsu
Copy link
Member

lostmsu commented Jun 17, 2020

BTW, @DanBarzilian , a personal thank you 10x! This finding and the fix for it just saved me after 3 workdays of chasing a semi-random segfault in tests. I don't know how much time I would have wasted trying to find this.

Funnily, while searching for (apparently) this issue, I stumbled and fixed another less impacting one: losttech@74e7d62

@lostmsu
Copy link
Member

lostmsu commented Jun 17, 2020

@filmor @koubaa we should mandate using *Reference types in new PRs. This bug would have been prevented by proper typing.

@koubaa
Copy link
Contributor

koubaa commented Jun 19, 2020

@lostmsu now it explains why the CI has been unreliable recently. Big whoops for missing this and yes more evidence to use Reference types. Good catch @DanBarzilian

@DanBarzilian DanBarzilian deleted the PR/fix-tp_repr-bug branch June 23, 2020 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lostmsu lostmsu lostmsu approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@DanBarzilian @dnfadmin @codecov-commenter @filmor @lostmsu @koubaa