Skip to content
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

Export logic code cleanup #80

Merged
merged 1 commit into from Jan 29, 2024
Merged

Export logic code cleanup #80

merged 1 commit into from Jan 29, 2024

Conversation

ghost
Copy link

@ghost ghost commented Jan 28, 2024

I looked at the stack trace with gdb after reproducing #67 to see a double free error:

#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=139885056115072)
    at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=139885056115072)
    at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=139885056115072, signo=signo@entry=6)
    at ./nptl/pthread_kill.c:89
#3  0x00007f3987187476 in __GI_raise (sig=sig@entry=6)
    at ../sysdeps/posix/raise.c:26
#4  0x00007f398716d885 in __GI_abort () at ./stdlib/abort.c:100
#5  0x00007f39871ce676 in __libc_message (action=action@entry=do_abort, 
    fmt=fmt@entry=0x7f3987320b77 "%s\n") at ../sysdeps/posix/libc_fatal.c:155
#6  0x00007f39871e5cfc in malloc_printerr (
    str=str@entry=0x7f39873236f0 "free(): double free detected in tcache 2")
    at ./malloc/malloc.c:5664
#7  0x00007f39871e80ab in _int_free (av=0x7f398735fc80 <main_arena>, 
    p=0x1faab050, have_lock=0) at ./malloc/malloc.c:4473
#8  0x00007f39871ea453 in __GI___libc_free (mem=<optimized out>)
    at ./malloc/malloc.c:3391
#9  0x00007f397f8126e2 in ?? ()
   from /path/to/game/addons/godotopenxrvendors/pico/.bin/libgodotopenxrpico.linux.template_debug.x86_64.so

I've noticed that remove_export_plugin frees the export plugin meaning that the memfree called after is throwing the error.

I can't test it, but I'm almost sure this fixes the error. I haven't worked with C++ yet, first time.

@m4gr3d m4gr3d added this to the 2.0.4 milestone Jan 28, 2024
@m4gr3d m4gr3d added the bug Something isn't working label Jan 28, 2024
@BastiaanOlij
Copy link
Member

Looking through the code, remove_export_plugin only removes the entry from the list of plugins, it should not free the object.

The real issue here is that plugins should be handed through references, we shouldn't be using memnew and memfree to create these objects. The result thus is that on the removal of the plugin from the array, the refcount hits 0, and the object is freed. But that's a fluke, not something to rely on.

To properly fix this, OpenXREditorExportPlugin *pico_export_plugin = nullptr; should become Ref<OpenXREditorExportPlugin>, and we should use pico_export_plugin.instantiate() and pico_export_plugin.unref() to instantiate and free these objects.

Similar changes should be applied to the other plugins.

@ghost
Copy link
Author

ghost commented Jan 28, 2024

Expected due to my very limited knowledge of C++. Effectively the build still has the issue, making that pr permitted to test my changes. Thanks for your understanding, going to update this pr with your recommended changes.

@m4gr3d m4gr3d added enhancement New feature or request and removed bug Something isn't working labels Jan 29, 2024
@m4gr3d m4gr3d modified the milestones: 2.0.4, 2.1.0 Jan 29, 2024
Copy link
Collaborator

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the changes and unfortunately they don't resolve #67.

I'll go ahead and merge the PR however because it's an enhancement on our current logic.

@m4gr3d m4gr3d merged commit e946c3a into GodotVR:master Jan 29, 2024
5 checks passed
@m4gr3d m4gr3d changed the title Fix double free Export logic code cleanup Jan 29, 2024
@m4gr3d m4gr3d modified the milestones: 2.1.0, 3.0.0 Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants