GNOME Bugzilla – Bug 733413
[PATCH] Undefined memory access in gdk-pixbuf-io.c
Last modified: 2014-11-21 15:37:52 UTC
Created attachment 281193 [details] [review] Fix undefined memory access In gdk-pixbuf-io.c, correct_prefix() contains the following line: if (strlen(*path) > 5 && strncmp (*path - 5, ".libs", 5) == 0) I am not sure what was the intent of subtracting 5 from *path, but it results in reading undefined memory 5 bytes before the beginning of the string: ~~Dr.M~~ Error #16: UNADDRESSABLE ACCESS: reading 0x029364fb-0x029364fc 1 byte(s) ~~Dr.M~~ # 0 libgdk_pixbuf-2.0-0.dll!get_file_formats [c:\src\gdk-pixbuf-2.30.3\gdk-pixbuf/gdk-pixbuf-io.c:343] ~~Dr.M~~ # 1 libgdk_pixbuf-2.0-0.dll!gdk_pixbuf_get_formats [c:\src\gdk-pixbuf-2.30.3\gdk-pixbuf/gdk-pixbuf-io.c:3151] ~~Dr.M~~ # 2 libgdk-3-0.dll!_gdk_win32_selection_init ~~Dr.M~~ # 3 libgdk-3-0.dll!_gdk_win32_windowing_init ~~Dr.M~~ # 4 libgdk-3-0.dll!gdk_win32_display_manager_init ~~Dr.M~~ # 5 libgobject-2.0-0.dll!g_type_create_instance ~~Dr.M~~ # 6 libgobject-2.0-0.dll!g_object_new_internal ~~Dr.M~~ # 7 libgobject-2.0-0.dll!g_object_newv ~~Dr.M~~ # 8 libgobject-2.0-0.dll!g_object_new ~~Dr.M~~ # 9 libgdk-3-0.dll!gdk_display_manager_get ~~Dr.M~~ #10 libgdk-3-0.dll!gdk_pre_parse_libgtk_only ~~Dr.M~~ #11 libgtk-3-0.dll!do_pre_parse_initialization ~~Dr.M~~ Note: @0:00:07.200 in thread 404 ~~Dr.M~~ Note: refers to 5 byte(s) before next malloc ~~Dr.M~~ Note: next higher malloc: 0x02936500-0x02936541 ~~Dr.M~~ Note: prev lower malloc: 0x02936460-0x029364e0 ~~Dr.M~~ Note: instruction: rep cmps %ds:(%esi) %es:(%edi) %esi %edi %ecx -> %esi %edi %ecx The attached patch removes the subtraction and compares the first 5 bytes of the string with ".libs" instead.
Review of attachment 281193 [details] [review]: I dont think the strlen check beforehand is needed. Also needs a commit message. Other then that, ok
Review of attachment 281193 [details] [review]: I agree, the strlen() is not needed. For a commit message, "Fix invalid memory access in Win32 initialization" will do.
Review of attachment 281193 [details] [review]: John, you'll need to update your patch as well before it's committed. ::: gdk-pixbuf/gdk-pixbuf-io.c.0 @@ -341,3 @@ { gchar *tem = NULL; - if (strlen(*path) > 5 && strncmp (*path - 5, ".libs", 5) == 0) That's probably supposed to be *path + strlen (*path) - 5, to check for the last 5 characters of the string being ".libs", no?
I assumed it was intended to check for the *first* 5 characters of the string being ".libs". However, it's been a few months and I don't remember why I thought that. Regardless, I'm not really interested in providing an updated patch, so consider my patch only a suggestion, and fix the bug however you see fit.
Created attachment 290825 [details] [review] Fix check if library is running inside build tree
Created attachment 290826 [details] [review] Fix check if library is running inside build tree Of course, g_str_has_suffix is a lot easier to read...
Review of attachment 290826 [details] [review]: Looks good, thanks!
commit d6c285a5e47964ab73836169cf919b0fa7a26186 Author: Robert Ancell <robert.ancell@canonical.com> Date: Mon Nov 17 15:35:54 2014 +1300 io: Fix check if library is running inside build tree The existing check was missing the length of the string so instead of reading the end of the string it read the memory before it. https://bugzilla.gnome.org/show_bug.cgi?id=733413