After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 544776 - [win32] binary registry may fail if glib is using a different c runtime
[win32] binary registry may fail if glib is using a different c runtime
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Windows
: Normal normal
: 0.10.21
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-07-25 22:58 UTC by Michael Smith
Modified: 2008-08-12 20:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix the bug (10.52 KB, patch)
2008-07-25 22:59 UTC, Michael Smith
committed Details | Review
Alternate solution (2.26 KB, patch)
2008-07-28 21:45 UTC, Ole André Vadla Ravnås
none Details | Review

Description Michael Smith 2008-07-25 22:58:20 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).
Comment 1 Michael Smith 2008-07-25 22:59:31 UTC
Created attachment 115277 [details] [review]
Fix the bug
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2008-07-26 12:19:46 UTC
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.
Comment 3 David Schleef 2008-07-28 05:11:14 UTC
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.
Comment 4 Michael Smith 2008-07-28 17:25:23 UTC
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.
Comment 5 Ole André Vadla Ravnås 2008-07-28 18:24:24 UTC
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.)
Comment 6 Ole André Vadla Ravnås 2008-07-28 21:45:40 UTC
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.
Comment 7 Michael Smith 2008-08-11 19:04:08 UTC
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.
Comment 8 Michael Smith 2008-08-11 19:04:46 UTC
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.
Comment 9 Ole André Vadla Ravnås 2008-08-12 20:37:38 UTC
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.