GNOME Bugzilla – Bug 544776
[win32] binary registry may fail if glib is using a different c runtime
Last modified: 2008-08-12 20:37:38 UTC
On win32, various things are local to the C runtime, including file descriptors. Thus, if glib and gstreamer are linked against different C runtimes (a common occurance), it's unsafe to open a file descriptor in glib, and use it from gstreamer. The binary registry uses g_mkstmp() to open a temp file, returning a file descriptor from glib, and then writes to it and closes it locally. This causes assertions in debug builds, and crashes elsewhere (if glib is using a different c runtime). The patch to come refactors some of the code so that the existing code is used for non-win32 platforms, and for win32, the binary registry is written to a memory buffer, then written to the file with g_file_set_contents() (which is entirely within glib).
Created attachment 115277 [details] [review] Fix the bug
The patch looks good. Is there a reason to not use the win32 code on linux too. Obviously it uses more memory and it does a lot of realloc (which could be optimized). But then writing in one go might be quicker in turn. So in the end I don't expect worse performance from the win32 code.
Why would anyone want to link against multiple C runtimes? That seems like a lose-lose situation. I think it would be best to fix the underlying problem that causes that to happen.
Stefan: people requested that I do it this way, because it increases peak memory usage - embedded users might care. David: I believe this is considered normal practice on win32, unfortunately. Hopefully someone who knows win32 better can answer more completely.
David: Unfortunately Windows doesn't have file descriptors as first class citizens, so they're implemented by the C run-time and thus considered private to it. This also applies to memory allocated by malloc(), it should always be free()d by a caller within the same executable module (DLL). A third example is errno, it should never be accessed across module boundaries (across different DLLs). The general rule of thumb is that if a library allocates a resource, it should also be responsible for freeing that resource, and if it's a handle it's only meaningful to the library. Compiling the whole stack with the same C-runtime is of course the optimum choice (footprint and performance-wise), but relying on it is a workaround and considered very bad practice. Some popular toolchains and their C-runtimes: MSVS 6: msvcrt.dll MSVS 2003: msvcr71.dll (and/or msvcr70.dll, don't remember which) MSVS 2005: msvcr80.dll MSVS 2008: msvcr90.dll mingw: depends on which version of mingw (one of msvcrt.dll and msvcr71.dll if memory serves) Add to this that each version of the C-runtime has at least four incompatible versions: Multi-threaded Debug (static) Multi-threaded (static) Multi-threaded DLL Debug (dynamic) Multi-threaded DLL (dynamic) (MSVS6 had two more, for single-threaded (legacy) applications.)
Created attachment 115456 [details] [review] Alternate solution The patch looks good to me. The only alternative approach that I can think of is to use GetTempFileName() followed by open(), and use g_mkstemp() for other platforms. Here's a quick and dirty mock-up, not tested with GLib built with a different CRT though. It's just a mock so I didn't bother with any error reporting (errno cannot be used like on non-Windows), and it's not really polished, just to spin some ideas around.
In the interests of getting this fixed, committed my patch. Ole: if you prefer yours, feel free to revert and commit a completed version of yours.
2008-08-11 Michael Smith <msmith@songbirdnest.com> * gst/gstregistrybinary.c: Don't use g_mkstmp() on win32, it's unsafe if glib is using a different libc. Fixes #544776.
I've been thinking about this one a bit on and off and I must say I prefer yours, I'm not a big fan of temporary files when it can be avoided.