GNOME Bugzilla – Bug 758738
Usage of GType properties causes crashes due to gulong/gpointer mismatch
Last modified: 2016-05-06 10:10:36 UTC
Created attachment 316385 [details] Gstreamer logs Here is the backtrace Program received signal SIGSEGV, Segmentation fault. 0x0000000063a6318d in type_node_check_conformities_UorL (have_lock=0, support_prerequisites=1, support_interfaces=1, iface_node=0xf1c3ee0e98, node=0xffffffffc5c38500) at gtype.c:3455 3455 gtype.c: No such file or directory. (gdb) bt
+ Trace 235766
the crash happen in an app that use gstreamer souphttpsrc element as soon as this code is called http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/ext/soup/gstsouphttpsrc.c?h=1.6#n932 ideas?
Could you do (gdb) print *pspec in frame 3 and (gdb) print *src in frame 10?
*** Bug 758739 has been marked as a duplicate of this bug. ***
(In reply to Tim-Philipp Müller from comment #1) > Could you do > > (gdb) print *pspec > > in frame 3 and > > (gdb) print *src > > in frame 10? sure (gdb) frame 3
+ Trace 235767
$2 = {element = {parent = {element = {object = {object = {g_type_instance = { g_class = 0x94a2dec890}, ref_count = 5, qdata = 0x0}, lock = { p = 0x0, i = {0, 0}}, name = 0x94a2deda10 "nvr_rec_src", parent = 0x94a2f3eab0, flags = 16448, control_bindings = 0x0, control_rate = 100000000, last_sync = 18446744073709551615, _gst_reserved = 0x0}, state_lock = {p = 0x94a4b28510, i = {0, 0}}, state_cond = {p = 0x0, i = {0, 0}}, state_cookie = 2, target_state = GST_STATE_PAUSED, current_state = GST_STATE_READY, next_state = GST_STATE_PAUSED, pending_state = GST_STATE_PAUSED, last_return = GST_STATE_CHANGE_ASYNC, bus = 0x94a4b25ca0, clock = 0x0, base_time = 0, start_time = 0, numpads = 1, pads = 0x94a4b27800, numsrcpads = 1, srcpads = 0x94a4b27a80, numsinkpads = 0, sinkpads = 0x0, pads_cookie = 1, _gst_reserved = { 0x0, 0x0, 0x0, 0x0}}, srcpad = 0x94a2ded660, live_lock = {p = 0x0, i = {0, 0}}, live_cond = {p = 0x0, i = {0, 0}}, is_live = 1, live_running = 0, blocksize = 4096, can_activate_push = 1, random_access = 0, clock_id = 0x0, segment = { flags = GST_SEGMENT_FLAG_NONE, rate = 1, applied_rate = 1, format = GST_FORMAT_BYTES, base = 0, offset = 0, start = 0, stop = 18446744073709551615, time = 0, position = 0, duration = 18446744073709551615, _gst_reserved = {0x0, 0x0, 0x0, 0x0}}, need_newsegment = 0, num_buffers = -1, num_buffers_left = -1, typefind = 0, running = 0, pending_seek = 0x0, priv = 0x94a2ded090, _gst_reserved = {0x0 <repeats 20 times>}}, ---Type <return> to continue, or q <return> to quit--- _gst_reserved = {0x0, 0x0, 0x0, 0x0}}, location = 0x94a586a140 "http://192.168.2.251:55080/stream11?want_rec_quality= 1", redirection_uri = 0x0, redirection_permanent = 0, user_agent = 0x94a2ded890 "GStreamer souphttpsrc ", automatic_redirect = 1, proxy = 0x0, user_id = 0x94a586a0d0 "admin", user_pw = 0x94a586a1b0 "password", proxy_id = 0x0, proxy_pw = 0x0, cookies = 0x0, context = 0x94a586a8f0, loop = 0x94a586a710, session = 0x0, session_io_status = GST_SOUP_HTTP_SRC_SESSION_IO_STATUS_IDLE, msg = 0x0, ret = GST_FLOW_OK, outbuf = 0x0, interrupted = 0, retry = 0, retry_count = 0, max_retries = 3, method = 0x0, got_headers = 0, have_size = 0, content_size = 0, read_position = 0, seekable = 0, request_position = 0, stop_position = 18446744073709551615, have_body = 0, keep_alive = 0, ssl_strict = 0, ssl_ca_file = 0x0, ssl_use_system_ca_file = 1, tls_database = 0x0, iradio_mode = 1, src_caps = 0x0, iradio_name = 0x0, iradio_genre = 0x0, iradio_url = 0x0, extra_headers = 0x0, log_level = SOUP_LOGGER_LOG_HEADERS, compress = 0, timeout = 15, mutex = {p = 0x0, i = {0, 0}}, request_finished_cond = { p = 0x0, i = {0, 0}}, http_headers_event = 0x0}
this is what I do in my app on souphttpsrc element, probably not relevant g_object_set(source,"location",this->mMediaUrl.constData(),"is-live",TRUE,"timeout",timeout,NULL); if (g_object_class_find_property(G_OBJECT_GET_CLASS(source),"ssl-strict") != NULL){ g_object_set(G_OBJECT(source),"ssl-strict",FALSE,NULL); } if (!this->mUsername.isEmpty()){ g_object_set(source,"user-id",this->mUsername.constData(),NULL); } if (!this->mPassword.isEmpty()){ g_object_set(source,"user-pw",this->mPassword.constData(),NULL); } gst_bin_add_many(GST_BIN(this->pipe),source,demuxer,NULL); if (!gst_element_link_many(source,demuxer,NULL)){ ....
my app "sometimes" works but most of the times crash with the above details
I wrote a really simple test app: gst_init(&argc,&argv); GstElement *source, *sink, *pipe; pipe = gst_pipeline_new("pipe"); source = gst_element_factory_make("souphttpsrc","http_src"); sink = gst_element_factory_make("fakesink","sink"); if (!source || !sink || !pipe){ fprintf(stderr,"Errore creazione elementi\n"); return 1; } g_object_set(source,"location","http://admin:password@192.168.2.251:12011/stream?want_rec_quality=1","is-live",TRUE,"timeout",(guint)10,"ssl-strict",FALSE,NULL); gst_bin_add_many(GST_BIN(pipe),source,sink,NULL); if (!gst_element_link_many(source,sink,NULL)){ fprintf(stderr,"Errore creazione elementi\n"); return 1; } gst_element_set_state(pipe,GST_STATE_PLAYING); fprintf(stderr,"pipeline avviata\n"); when does not crash it print this warning: GLib-GObject-WARNING **: value "((gpointer) 000000CB31E0ADB0)" of type 'GType' is invalid or out of range for property 'add-feature-by-type' of type 'GType'
my fix, probably workaround, was to remove this line from souphttpsrc gstreamer plugin (I don't need a proxy) http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/ext/soup/gstsouphttpsrc.c?h=1.6#n936
How about a small test program that just uses libsoup (a stripped-down version of gst_soup_http_src_session_open() with the same values)?
I can try the next week. However seems quite clear that SOUP_SESSION_ADD_FEATURE_BY_TYPE lead to crash on *some* windows variants, for example it works fine on windows 7 but crash on windows 8 (both x64)
This looks like possibly a gobject-level problem with GType-valued properties. Is everything (glib, libsoup, gstreamer, your app) compiled with the same toolchain? It looks like the definition of GType is environment-dependent: #if GLIB_SIZEOF_SIZE_T != GLIB_SIZEOF_LONG || !defined __cplusplus typedef gsize GType; #else /* for historic reasons, C++ links against gulong GTypes */ typedef gulong GType; #endif so if some parts got one definition and other parts got the other, then things would break.
(In reply to Dan Winship from comment #10) > This looks like possibly a gobject-level problem with GType-valued > properties. > > Is everything (glib, libsoup, gstreamer, your app) compiled with the same > toolchain? It looks like the definition of GType is environment-dependent: > > #if GLIB_SIZEOF_SIZE_T != GLIB_SIZEOF_LONG || !defined __cplusplus > typedef gsize GType; > #else /* for historic reasons, C++ links against gulong GTypes */ > typedef gulong GType; > #endif > > so if some parts got one definition and other parts got the other, then > things would break. glib, libsoup and gstreamer are compiled using cerbero. My app is compiled using visual studio, this should be supported by gstreamer distribution, Tim?
(compiled using cerbero == mingw/gcc fwiw) Yes, this should work, although there might be interesting issues if you pass file descriptors from your app to GStreamer and things like that, but this is not what's going on here as far as I can tell. Since both libsoup and souphttpsrc have been compiled using cerbero/mingw, there should be no problem between those two. I'm not aware of any problems with GType and MSVC either, people do that quite a bit, that they build their own applications in Visual Studio and link against the pre-built binaries. How about a little test app that just does: GType type = SOUP_TYPE_PROXY_RESOLVER_DEFAULT; g_print ("%d\n", (int) sizeof (GType)); g_print ("%d\n", (int) sizeof (void *)); g_print ("%s\n", g_type_name (type));
(In reply to Tim-Philipp Müller from comment #12) > (compiled using cerbero == mingw/gcc fwiw) > > Yes, this should work, although there might be interesting issues if you > pass file descriptors from your app to GStreamer and things like that, but > this is not what's going on here as far as I can tell. > > Since both libsoup and souphttpsrc have been compiled using cerbero/mingw, > there should be no problem between those two. I'm not aware of any problems > with GType and MSVC either, people do that quite a bit, that they build > their own applications in Visual Studio and link against the pre-built > binaries. > > How about a little test app that just does: > > GType type = SOUP_TYPE_PROXY_RESOLVER_DEFAULT; > > g_print ("%d\n", (int) sizeof (GType)); > g_print ("%d\n", (int) sizeof (void *)); > g_print ("%s\n", g_type_name (type)); ok, this app should be compiled with visual studio and not cerbero, right? I'll try later, thanks
(In reply to Tim-Philipp Müller from comment #12) > (compiled using cerbero == mingw/gcc fwiw) > > Yes, this should work, although there might be interesting issues if you > pass file descriptors from your app to GStreamer and things like that, but > this is not what's going on here as far as I can tell. > > Since both libsoup and souphttpsrc have been compiled using cerbero/mingw, > there should be no problem between those two. I'm not aware of any problems > with GType and MSVC either, people do that quite a bit, that they build > their own applications in Visual Studio and link against the pre-built > binaries. > > How about a little test app that just does: > > GType type = SOUP_TYPE_PROXY_RESOLVER_DEFAULT; > > g_print ("%d\n", (int) sizeof (GType)); > g_print ("%d\n", (int) sizeof (void *)); > g_print ("%s\n", g_type_name (type)); seems good: C:\Users\Administrator>C:\Users\Administrator\Desktop\Condivisa\Deploy-x64\CrashTest.exe WARNING: no real random source present! 8 8 SoupProxyResolverDefault and then crash
Created attachment 316486 [details] sample app that crash on windows this is the app that produce the previous output
Created attachment 316487 [details] test app that crash on windows
(In reply to Tim-Philipp Müller from comment #8) > How about a small test program that just uses libsoup (a stripped-down > version of gst_soup_http_src_session_open() with the same values)? If I modify the test program to something like this: GMainContext *context = g_main_context_new(); g_print("try to create soup session\n"); SoupSession *session = soup_session_async_new_with_options (SOUP_SESSION_ASYNC_CONTEXT,context, SOUP_SESSION_USER_AGENT, "user_agent", SOUP_SESSION_TIMEOUT, (guint)10, SOUP_SESSION_SSL_STRICT, FALSE, SOUP_SESSION_ADD_FEATURE_BY_TYPE, SOUP_TYPE_PROXY_RESOLVER_DEFAULT,NULL); g_print("soup session created %p\n",session); I can reproduce the crash
even something more basic like this: SoupSession *session = soup_session_new_with_options (SOUP_SESSION_ADD_FEATURE_BY_TYPE, SOUP_TYPE_CONTENT_SNIFFER,NULL); copied from here: https://developer.gnome.org/libsoup/stable/libsoup-client-howto.html will crash. The crash seem related to SOUP_SESSION_ADD_FEATURE_BY_TYPE
and finally if I rename the same app from main.cpp to main.c and I cross compile with cerbero this way: x86_64-w64-mingw32-gcc main.c -o CrashTestCross.exe $(pkg-config --cflags --libs gstreamer-1.0 libsoup-2.4) no crash happen C:\Users\Administrator>C:\Users\Administrator\Desktop\Condivisa\Deploy-x64\Crash TestCross.exe 8 8 SoupProxyResolverDefault try to create soup session soup session created 0000000000439120
and yet more strange if I create a soup session like this: SoupSession *session = soup_session_async_new_with_options (SOUP_SESSION_ASYNC_CONTEXT,context, SOUP_SESSION_USER_AGENT, "user_agent",SOUP_SESSION_TIMEOUT, (guint)10, SOUP_SESSION_SSL_STRICT, FALSE, NULL); so the same code as in souphttpsrc but without SOUP_SESSION_ADD_FEATURE_BY_TYPE, SOUP_TYPE_PROXY_RESOLVER_DEFAULT then this code works without crash: g_print("session created %p\n",session); soup_session_add_feature_by_type(session, SOUP_TYPE_CONTENT_DECODER); g_print("added content decoder\n"); soup_session_remove_feature_by_type(session, SOUP_TYPE_CONTENT_DECODER); g_print("removed content decoder\n"); so "add-feature-by-type" this time works really no more ideas, what do you think about? P.S. after my modification in comment 7 my app is running since 54 hours with no crash
(In reply to Nicola from comment #18) > The crash seem related to SOUP_SESSION_ADD_FEATURE_BY_TYPE (In reply to Nicola from comment #20) > so "add-feature-by-type" this time works Right. The problem is specifically with demarshalling a GType passed to a varargs function. So soup_session_add_feature_by_type() works, but soup_session_new_with_options() doesn't. (In reply to Nicola from comment #19) > and finally if I rename the same app from main.cpp to main.c and I cross > compile with cerbero this way: > > x86_64-w64-mingw32-gcc main.c -o CrashTestCross.exe $(pkg-config --cflags > --libs gstreamer-1.0 libsoup-2.4) > > no crash happen So this suggests something along the lines of what I said in comment 10. Can you add printfs for sizeof(gulong) and the values of GLIB_SIZEOF_SIZE_T and GLIB_SIZEOF_LONG to your test? (It seems like the problem might be that GLIB_SIZEOF_LONG is somehow ending up as 8 but sizeof(gulong) is 4.)
(In reply to Dan Winship from comment #21) > (In reply to Nicola from comment #18) > > The crash seem related to SOUP_SESSION_ADD_FEATURE_BY_TYPE > > (In reply to Nicola from comment #20) > > so "add-feature-by-type" this time works > > Right. The problem is specifically with demarshalling a GType passed to a > varargs function. So soup_session_add_feature_by_type() works, but > soup_session_new_with_options() doesn't. > > (In reply to Nicola from comment #19) > > and finally if I rename the same app from main.cpp to main.c and I cross > > compile with cerbero this way: > > > > x86_64-w64-mingw32-gcc main.c -o CrashTestCross.exe $(pkg-config --cflags > > --libs gstreamer-1.0 libsoup-2.4) > > > > no crash happen > > So this suggests something along the lines of what I said in comment 10. > > Can you add printfs for sizeof(gulong) and the values of GLIB_SIZEOF_SIZE_T > and GLIB_SIZEOF_LONG to your test? (It seems like the problem might be that > GLIB_SIZEOF_LONG is somehow ending up as 8 but sizeof(gulong) is 4.) you are right, this code: g_print("sizeof(gulong): %ld\n",sizeof(gulong)); g_print("GLIB_SIZEOF_SIZE_T: %d\n",GLIB_SIZEOF_SIZE_T); g_print("GLIB_SIZEOF_LONG: %d\n",GLIB_SIZEOF_LONG); print these results: sizeof(gulong): 4 GLIB_SIZEOF_SIZE_T: 8 GLIB_SIZEOF_LONG: 4
(In reply to Nicola from comment #22) > sizeof(gulong): 4 > GLIB_SIZEOF_SIZE_T: 8 > GLIB_SIZEOF_LONG: 4 OK, that's *not* what I was thinking before. That's correct and consistent. (I was thinking sizeof(gulong) and GLIB_SIZEOF_LONG might be different.) Given that GLIB_SIZEOF_SIZE_T != GLIB_SIZEOF_LONG, gtype.h will define GType as gsize regardless of C vs C++. So I don't know exactly what's happening, but *somehow* you're ending up with some sort of mismatch in what different parts of your code think sizeof(GType) is.
on linux 64 the size is 8. If I cross compile for windows x64 I get exactly the same results posted above for the test app compiled with visual studio.
the really strange thing is that the same test app works with no crash on windows 7 64bit and crash very often on windows 8 64bit. Both machine have the same visual c++ runtime version. My workaround seems to be fine, but to be sure that my app works on all windows versions seems that I have to build everything from cerbero/mingw or use DLLImport No more ideas? thanks
the crash seeems solved using DYNAMICBASE:NO linker flags, this flag is active by default
I did several tests on different windows versions (7,8,10) and I can confirm that passing DYNAMICBASE:NO to visual studio linker solve the crash on all tested versions. With defaults linker flags, and so DYNAMICBASE enabled, the test app works only if it runs on the same windows version that is used for its compilation. dynamicbase seems broken and/or not fully working with mingw: http://sourceforge.net/p/mingw-w64/mailman/mingw-w64-public/thread/CAGFXeQKg0J+ax0Dj-cBXGGrwWT15K-aRsWWaBN6wWSDUtRHoLg@mail.gmail.com/ The crash seems to happen only with varargs.
*** Bug 763962 has been marked as a duplicate of this bug. ***
Nicola, can you test the patch in bug #765031? If all our guesses are correct, this should at least work around the problem until we really understand it. Question now is what DYNAMICBASE is exactly doing and why/how it interferes with GType passing via varargs. It might also be fixed by a newer toolchain, we're considering updating the Windows toolchain for GStreamer soonish.
Created attachment 325991 [details] [review] gparamspecs: GTypes are stored in v_pointer, not v_long v_long is 32 bits on Win64, v_pointer is 64 bits. On most other platforms the size of long and pointer is the same, so it's usually not a problem.
I confirmed that this patch fixes it for the GES case at least.
Here the weird conditions around definition of GType. That does not seem right to me, as it means GType size may vary on the same platform dependind if it's C or C++. #if GLIB_SIZEOF_SIZE_T != GLIB_SIZEOF_LONG || !defined __cplusplus typedef gsize GType; #else /* for historic reasons, C++ links against gulong GTypes */ typedef gulong GType; #endif
Yes that looks very bad but is unrelated to this bug fortunately. But basically the GType definition means that it will fail interoperability between C and C++ code on Win64 as gsize and gulong have different sizes. I assume the current definition has historical reasons. It was gulong in the beginning, then it was changed to gsize, then people noticed that this breaks the C++ ABI (having a gsize will result in different symbol mangling than a gulong I guess), and then this compromise was chosen. But that's just guessing. However this also means that we can't possibly change anything here now without breaking the C++ ABI. OTOH if we don't change anything, GLib is basically unusable from C++ on Win64.
Also I'm not sure why the original problem only broke on Windows 10 with DYNAMICBASE:YES. Why would the GTypes have bits set in the upper 32 bits in that case, but not in the other?
See that breaking the abi on windows is not such a big deal since you always endup building everything by yourself.
Hi Sebastian, thanks for the patch, tomorrow I'll try to find some time to test it. I'm sure it works, thanks! For info I'm using gstreamer from a c++ app (qt based), for me the problem happen on windows 8/10 64 bits and not on windows 7 64 bits. The application was compiled on windows 7 64bits using qtcreator with visual studio compiler, adding DYNAMICBASE:NO seems to solve the issue (not widely tested)
It should only break C++ on platforms where sizeof(long) == sizeof(size_t)... and where gsize and gulong are resulting in different symbol mangling although the have the same size. That's not even Win64 (the sizes are different), and not with g++ in general (the mangling only considers the basic types, not any typedefs). So not sure what it would break... but the code probably has reasons. Someone needs to read the logs :)
I confirm that the patch fix the reported issue, thanks! Do you want any additional test? I have a c++ app that can generate the logs you want on win64 :)
What should happen with the commit now that fixes it for Win64? Getting that in would be a good start, then we can worry about the remaining problem (which is also unrelated to Windows).
Review of attachment 325991 [details] [review]: I'd prefer to introduce a v_uintptr or something to the union and use that.
(In reply to Allison Ryan Lortie (desrt) from comment #40) > Review of attachment 325991 [details] [review] [review]: > > I'd prefer to introduce a v_uintptr or something to the union and use that. Me too, but that probably would have to wait for next major GLib release then and it would be good to have this crash fixed on Windows before that :) The way how the patch does it now is the same as elsewhere already: https://git.gnome.org/browse/glib/tree/gobject/gvaluetypes.c#n1188 https://git.gnome.org/browse/glib/tree/gobject/gvaluetypes.c#n1207 I can prepare an additional patch on top of the above one for master, but I would prefer to have it merged in two steps so that the above patch without new API can be cherry-picked for the next stable release. Question is always which integer types you all want in GValue. uintptr? size_t? ssize_t? All of them, some subset of them, ...? They can all be different types in theory.
As far as I am concerned, sizeof(gsize) == sizeof(gpointer). A great number of bad things would happen if this were to stop being the case. As for the exact type we use in the union, I doubt that it matters -- even for C++. This union is also semi-private -- people aren't supposed to touch it outside of GLib... even in marshallers, accessing these fields is considered to be slightly evil. I agree with your strategy, though. Let's do it your way for now. Thanks.
Review of attachment 325991 [details] [review]: Okay. Go ahead with this one and we'll do the other one later.
Attachment 325991 [details] pushed as 4e3cd88 - gparamspecs: GTypes are stored in v_pointer, not v_long
For everyone who's wondering why it would crash on Windows 8 and above and not on 7, the allocator on 8 and above tends to give 64-bit pointers that have non-zero upper 8 bits more often that the allocator on 7. So truncating a pointer has has a higher chance of breaking it on 8 and above.