Skip to content

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.

2 participants