Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 17, 2020
Merged

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

merged 1 commit into from
Jun 17, 2020

Conversation

DanBarzilian
Copy link
Contributor

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