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

Fix leak of gst promise with custom change func #175

Merged
merged 3 commits into from
Apr 15, 2020

Conversation

kezhuw
Copy link
Contributor

@kezhuw kezhuw commented Nov 1, 2019

Synchronize signatures of GstPromiseAPI.gst_promise_new_with_change_func
and GstPromiseAPI.ptr_gst_promise_new_with_change_func to C's version,
so that it would not get unknown user_data and notify, otherwise
we will get coredump in gst_promise_free.

@neilcsmith-net
Copy link
Member

Thanks! Will review ASAP.

@neilcsmith-net
Copy link
Member

Not sure if the changes to the lowlevel mappings are required here?! You're also changing the version of Natives.initializer in use - potentially to the correct one.

@kezhuw
Copy link
Contributor Author

kezhuw commented Nov 5, 2019

The old invocation of Native.initializer(ptr, false, false) is where memory leak introduced. Following code creates a not owned promise in java.

Promise promise = new Promise(new Promise.PROMISE_CHANGE() {
    @Override
    public void onChange(Promise promise) {
    }
});

GstPromise * gst_promise_new_with_change_func (GstPromiseChangeFunc func, gpointer user_data, GDestroyNotify notify) has two additional parameters, while its java corresponding GstPromiseAPI.gst_promise_new_with_change_func doesn't. When we construct promise using java version, we pass only one argument, C version got random user_data, notify from registers or stacks. Thus in gst_promise_free we may got SIGBUS or SIGSEGV due to random notify.

Current version got no coredump due to memory leak, aka. no gst_promise_free at all.

Synchronize signatures of `GstPromiseAPI.gst_promise_new_with_change_func`
and `GstPromiseAPI.ptr_gst_promise_new_with_change_func` to C's version,
so that it would not get unknown `user_data` and `notify`, otherwise
we will get coredump in `gst_promise_free`.
@neilcsmith-net neilcsmith-net added this to the 1.2 milestone Apr 8, 2020
@neilcsmith-net neilcsmith-net merged commit 0e56e50 into gstreamer-java:master Apr 15, 2020
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.

None yet

2 participants