-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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.
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
Fix VisualShader connection use after free. #84832
Fix VisualShader connection use after free. #84832
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO you can use a more comprehensive commit message like your pr title
79b8838
to
f0d58d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably use some refactoring to avoid doing E->get()
a bunch of times, but looks fine as a quick fix.
Sure, I can refactor it. |
f0d58d0
to
a3d37ef
Compare
I've refactored the code as a second commit. I can squish them if you prefer. |
Sorry if I wasn't clear, but that wasn't necessary and I didn't mean for you to do it. You should probably remove the second commit. |
a3d37ef
to
1e566f9
Compare
All good. I'm new and not sure of the how much to put into a commit yet. |
And by removing it I meant removing it, not squashing it. We prefer squashed commits for PRs, but in this case I was asking you to revert your PR to the original change as it was already approved and sufficient. |
1e566f9
to
a30dc67
Compare
Restored back to the original point. Will submitted another PR once this is upstream. |
I don't want to discourage you, but I just wanted to make it clear that that was just my musings. It's not really necessary and you've already fixed the problem itself. Maybe a slightly reworked code could be better for future maintenance, but it's a minor thing, which is why we approved your PR as it was and didn't ask for any changes. Thanks for humoring me though! If you wish to contribute more, there are many more bugs to tackle in this issue tracker 🙂 |
Thanks! And congrats for your first merged Godot contribution 🎉 |
Coverity has picked up this as a "Use after free", the object E is deteled bu erase but is used straight after multiple times e->get().
Moving to the end of the if seems to make the most sense.