GNOME Bugzilla – Bug 761916
gstinfo: Make GST_DEBUG_PAD_NAME "MT crash safe"
Last modified: 2018-11-03 12:33:01 UTC
Created attachment 320952 [details] [review] test and fix The pad may be unparented while this macro is called which could result in a segfault. The new macro protects against this. The parent may still be disposed while the macro is called, but this will not result in a crash (but the parent name may be garbage). Using gst_pad_get_parent () is undesirable since it takes the object lock. The patch take advantage of compound expressions available as a C extension in GCC and some other compilers.
Comment on attachment 320952 [details] [review] test and fix Instead of using a gcc extension could we just use an inline function instead?
The issue with an inline function is that we cannot return "''" from it when the parent is NULL since it will go out of scope when returning. But if we change the behavior of GST_DEBUG_PAD_NAME to return "(NULL):src" instead of "'':src" when the pad has no parent it should work and definitely be nicer. I can try and submit a patch.
Created attachment 320978 [details] [review] alternative fix using inline function I'm not sure how to implement this safely with a function. As mentioned in previous comment the function cannot return a string that goes out of scope. If it returns NULL instead when there is no parent and we use GST_PTR_NULL or similar to try to make it safe, the function needs to be called twice (when assuming no compiler optimizations): Once for checking if name is not NULL and then again for getting the name which in the meantime could have been set to NULL. So in this case the GST_DEBUG_PAD_NAME can potentially return NULL which we want to avoid. That said, this patch at least does not use GCC extensions and is still an improvement over the existing macro that segfaults in the macro itself.
> The issue with an inline function is that we cannot > return "''" from it when the parent is NULL since it > will go out of scope when returning. I don't think this is true? As far as I'm aware it's perfectly fine to return a string literal from a function, its lifetime is basically the same as a static variable.
Created attachment 321100 [details] [review] fix and test using inline function Tim, you're right, of course. Since string literals are stored in the read-only data segment it's ok to return a pointer to it. A much better fix.
I wonder if we should go even one step further and inline more/most of the macro? That might save calling the current inline function twice because of GST_STR_NULL. Don't know if it's worth it or not, just a thought.
Created attachment 321346 [details] [review] fix and test using inline function Updated the patch so that GST_STR_NULL is called inside the function in order to call the _gst_object_parent_name() only once.
Review of attachment 321346 [details] [review]: ::: gst/gstinfo.h @@ +234,3 @@ + + /* parent may be disposed, but accessing name will not crash */ + name = GST_OBJECT_NAME (parent); Why will this not crash? Maybe between getting the parent and this line here a lot of other stuff happened in another thread, and dereferencing parent now leads to a crash because the page is not owned by the process anymore? And even otherwise, as you say it might contain garbage. Garbage that might not contain any \0 until the end of the page, which again would cause a crash then. I wouldn't call this MT safe still :) Even if it is better now.
Valid points. Let's call it "less prone to crash", change the comment and keep the FIXME that's already there? Without locking there's just so much one can do when it comes to MT safety.
Bit undecided what to do with this. It avoids one problem, but there are more left. I wonder if it wouldn't make sense to do things more properly here (incurring a small performance impact), i.e get parent with lock (and a ref then), copy name into g_alloca()-allocated memory or such (would need a max-name-length limitation then, which should be ok). Arguably this is only used when debug logging is actually active, so not in the case where it's not, and when logging is active a teeny bit of extra overhead won't matter that much, won't it?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/156.