FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Increase ob's ref count in tp_repr to avoid accidental free by DanBarzilian · Pull Request #1160 · pythonnet/pythonnet · GitHub

/ pythonnet Public

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

Merged
lostmsu merged 1 commit into
pythonnet:masterfrom
DanBarzilian:PR/fix-tp_repr-bug
Jun 17, 2020
ConversationCommits (1)ChecksFiles changed
Merged

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

Conversation

DanBarzilian commented Jun 17, 2020

Copy link
Copy Markdown
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 commented Jun 17, 2020 •
edited
Loading

Copy link
Copy Markdown


All CLA requirements met.

DanBarzilian mentioned this pull request Jun 17, 2020
Merged
4 tasks

codecov-commenter commented Jun 17, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@           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 requested a review from lostmsu June 17, 2020 09:25

filmor commented Jun 17, 2020

Copy link
Copy Markdown
Member

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

lostmsu commented Jun 17, 2020

Copy link
Copy Markdown
Member

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

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

lostmsu commented Jun 17, 2020

Copy link
Copy Markdown
Member

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 commented Jun 17, 2020

Copy link
Copy Markdown
Member

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

koubaa commented Jun 19, 2020

Copy link
Copy Markdown
Contributor

@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 deleted the PR/fix-tp_repr-bug branch June 23, 2020 08:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Back | FazBrowse Home | New Git URL