commit e7523ed41271e48a909011b8598d496c1be642e2 Author: Jehan Date: Mon Nov 4 00:04:40 2024 +0100 Issue #11822: fix double-free in edge cases of broken XCF. A patch was originally contributed by Andrzej Hunt in #11822 (cf. 0002-xcf-fix-channel-s-reference-counts.patch in the report). The diagnostic of the double PROP_SELECTION issue is right, but not the fix which was over-reffing, hence leaking channels and buffers, in the normal cases, just to avoid double-free in broken edge cases. The other issue is not possible though (unreffing the image's selection when encountering an error in xcf_load_channel()) because we explicitly check it it's the image mask AFAICS. I added a second test which was not double-freeing yet which deserves a bit of stderr messaging: when 2 different channels have PROP_SELECTION set. Relevant text from the commit message originally contributed by Andrzej Hunt is the following (diagnostic and ASAN output still of interest): ---------------- xcf_load_channel creates a new channel using gimp_channel_new. This channel has a floating reference (because GimpChannel is a subclass of GimpItem, and gimp_item_init uses g_object_force_floating()). Next, three different scenarios can occur: - xcf_load_channel_props does nothing, and we either return channel, OR in the error case we g_object_unref (channel) which frees channel. The returned channel is either silently dropped (in the case where it's already been set as the mask), or added to the image using gimp_image_add_channel if not (which sinks the floating reference). - xcf_load_channel_props encounters a single PROP_SELECTION. We create a selection using gimp_selection_new (which again has a floating reference), transfer ownership of the new selection to the image using gimp_image_take_mask(), free the old channel, and finally set channel to point to this new selection. Back in xcf_load_channel, IF we hit the error case, we call g_object_unref (channel), which frees the new selection - but we're still using it as the image's mask, meaning we could eventually hit a use-after-free whenever someone reads the mask. - xcf_load_channel_props encounters 2 PROP_SELECTION's. After the first PROP_SELECTION, channel is pointing to the image mask, which has reference count == 1 (as explained above). When we hit the second PROP_SELECTION: we create another new selection, followed by calling gimp_image_take_mask() again. gimp_image_take_mask() call g_object_unref() on the old mask, which frees it - but channel is still pointing to this mask. We then call g_object_unref() on channel, which is effectively a double-free. We fix this by making sure to always ref_sink whatever object is put into channel. gimp_image_take_mask also calls ref_sink, which means we'll now bump the refcount up to 2 when the channel is being used as the image's mask (and drop back to 1 if the mask is replaced, and down to 0 when channel is unref'd). See also ASAN output below from the 2x PROP_SELECTION scenario: ==6381==ERROR: AddressSanitizer: heap-use-after-free on address 0x6150000047d0 at pc 0x7fb5531ef31b bp 0x7ffe81e86cb0 sp 0x7ffe81e86ca8 READ of size 8 at 0x6150000047d0 thread T0 #0 0x7fb5531ef31a in g_type_check_instance_cast /home/ahunt/git/glib/_build/../gobject/gtype.c:4117:26 #1 0xb2346b in xcf_load_channel_props /home/ahunt/git/gimp/app/xcf/xcf-load.c:1742:41 #2 0xb1a3cc in xcf_load_channel /home/ahunt/git/gimp/app/xcf/xcf-load.c:2219:9 #3 0xb147eb in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:653:17 #4 0xb121bb in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19 #5 0x619ead in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:50:17 #6 0x51d414 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15 #7 0x507092 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:323:6 #8 0x50d400 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:856:9 #9 0x537452 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10 #10 0x7fb551e7c349 in __libc_start_main (/lib64/libc.so.6+0x24349) #11 0x4e0829 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120 0x6150000047d0 is located 336 bytes inside of 504-byte region [0x615000004680,0x615000004878) freed by thread T0 here: #0 0x5e8612 in free /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:127:3 #1 0x7fb552d9ce08 in g_free /home/ahunt/git/glib/_build/../glib/gmem.c:199:3 #2 0x7fb552dc7a6b in g_slice_free1 /home/ahunt/git/glib/_build/../glib/gslice.c:1183:7 #3 0x7fb5531e7b04 in g_type_free_instance /home/ahunt/git/glib/_build/../gobject/gtype.c:2008:5 #4 0x7fb5531bfe3a in g_object_unref /home/ahunt/git/glib/_build/../gobject/gobject.c:3604:11 #5 0xd4d4d4 in gimp_image_take_mask /home/ahunt/git/gimp/app/core/gimpimage.c:3267:5 #6 0xb23438 in xcf_load_channel_props /home/ahunt/git/gimp/app/xcf/xcf-load.c:1739:13 #7 0xb1a3cc in xcf_load_channel /home/ahunt/git/gimp/app/xcf/xcf-load.c:2219:9 #8 0xb147eb in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:653:17 #9 0xb121bb in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19 #10 0x619ead in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:50:17 #11 0x51d414 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15 #12 0x507092 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:323:6 #13 0x50d400 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:856:9 #14 0x537452 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10 #15 0x7fb551e7c349 in __libc_start_main (/lib64/libc.so.6+0x24349) previously allocated by thread T0 here: #0 0x5e887d in malloc /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x7fb552d9ccf2 in g_malloc /home/ahunt/git/glib/_build/../glib/gmem.c:106:13 #2 0x7fb552dc72e0 in g_slice_alloc /home/ahunt/git/glib/_build/../glib/gslice.c:1072:11 #3 0x7fb552dc78ae in g_slice_alloc0 /home/ahunt/git/glib/_build/../glib/gslice.c:1098:18 #4 0x7fb5531e6e0a in g_type_create_instance /home/ahunt/git/glib/_build/../gobject/gtype.c:1911:17 #5 0x7fb5531c215e in g_object_new_internal /home/ahunt/git/glib/_build/../gobject/gobject.c:1945:24 #6 0x7fb5531c1d1f in g_object_new_valist /home/ahunt/git/glib/_build/../gobject/gobject.c:2288:16 #7 0x7fb5531c0e8b in g_object_new /home/ahunt/git/glib/_build/../gobject/gobject.c:1788:12 #8 0xdb7260 in gimp_item_new /home/ahunt/git/gimp/app/core/gimpitem.c:722:10 #9 0xce1668 in gimp_drawable_new /home/ahunt/git/gimp/app/core/gimpdrawable.c:1067:14 #10 0xe283e9 in gimp_selection_new /home/ahunt/git/gimp/app/core/gimpselection.c:626:13 #11 0xb2342a in xcf_load_channel_props /home/ahunt/git/gimp/app/xcf/xcf-load.c:1735:15 #12 0xb1a3cc in xcf_load_channel /home/ahunt/git/gimp/app/xcf/xcf-load.c:2219:9 #13 0xb147eb in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:653:17 #14 0xb121bb in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19 #15 0x619ead in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:50:17 #16 0x51d414 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15 #17 0x507092 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:323:6 #18 0x50d400 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:856:9 #19 0x537452 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10 #20 0x7fb551e7c349 in __libc_start_main (/lib64/libc.so.6+0x24349) SUMMARY: AddressSanitizer: heap-use-after-free /home/ahunt/git/glib/_build/../gobject/gtype.c:4117:26 in g_type_check_instance_cast Shadow bytes around the buggy address: 0x0c2a7fff88a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c2a7fff88b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa 0x0c2a7fff88c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c2a7fff88d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c2a7fff88e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd =>0x0c2a7fff88f0: fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd 0x0c2a7fff8900: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa 0x0c2a7fff8910: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c2a7fff8920: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c2a7fff8930: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c2a7fff8940: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==6381==ABORTING ( crash-c35bcae86d35ce7d0cd8ffcb41a470f37354e018 ) diff --git a/app/xcf/xcf-load.c b/app/xcf/xcf-load.c index 1fb0752832..3b1bcc872a 100644 --- a/app/xcf/xcf-load.c +++ b/app/xcf/xcf-load.c @@ -2176,6 +2176,28 @@ xcf_load_channel_props (XcfInfo *info, continue; } + if (*channel == gimp_image_get_mask (image)) + { + /* PROP_SELECTION was already seen once for this + * channel. Let's silently ignore the second identical + * property to avoid a double free. + */ + continue; + } + else if (gimp_image_get_mask (image) != NULL && + ! gimp_channel_is_empty (gimp_image_get_mask (image))) + { + /* This would happen when PROP_SELECTION was already set + * on a previous channel. This is a minor case of data + * loss (we don't know which selection was the right one + * and we drop the non-first ones), and also means it's + * a broken XCF, though it's not a major bug either. So + * let's go with a stderr print. + */ + g_printerr ("PROP_SELECTION property was set on 2 channels (skipping)\n"); + continue; + } + /* We're going to delete *channel, Don't leave its pointer * in @info. See bug #767873. */