Skip to content

Fix dangling pointer in custom Reference _new#401

Closed
sheepandshepherd wants to merge 1 commit intogodotengine:masterfrom
sheepandshepherd:new_ref_fix
Closed

Fix dangling pointer in custom Reference _new#401
sheepandshepherd wants to merge 1 commit intogodotengine:masterfrom
sheepandshepherd:new_ref_fix

Conversation

@sheepandshepherd
Copy link
Contributor

@sheepandshepherd sheepandshepherd commented Apr 25, 2020

GODOT_CLASS's _new() frees the pointer it's supposed to return:

Name *instance = godot::as<Name>(script->new_());
//                │               │
//                │               └─> returns Variant: ref-count is 1
//                └─> returns Name*: ref-count drops to 0 when the Variant is destroyed

This PR inserts a reference() after the NativeScript::new_() call.

It does the same thing as #346, only at the location actually causing the dangling pointer, rather than in NativeScript::new_(). That function already works correctly, returning with a ref-count of 1.

Fixes #343 and #397

As discussed in #215, #346, and this comment, there's now a memory leak of the returned Reference because Ref(T *) increases its ref-count to 2. Another fix such as #350 will be needed in addition to this.

@BastiaanOlij
Copy link
Collaborator

@Zylann Is your PR an alternative to this or will this remain valid even after we apply your fix?

@Zylann
Copy link
Collaborator

Zylann commented Jun 2, 2020

@BastiaanOlij my fix solves the same problem without the side-effect, so I think it won't be needed anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A problem with the Ref

4 participants