I broke MR !1660 commits into 2 commits so that the strings are added
now (freeing this up for the upcoming string freeze), with this commit.
A next commit will be needed when we'll have the proper version update
infrastructure on GEGL side, because right now, all we can do when
filter versions don't match is ignoring the filter.
In earlier development, we accidentally
allowed users to save gegl:nop filters.
Since they are valid operations, they were
not triggering our unsupported operations
delete code. This patch adds a second
check to see if the operation is a nop, and
deletes it as well.
This patch also adds a check to make sure
a filter is in the filter stack before reordering
it. This was revealed by the nop bug, so
it makes sense to patch both at once.
Rather than trying to implement full i18n plural support, we just remove
this failed attempt from the past. The fact is that to get proper
support, we'd basically need to reimplement a Gettext-like plural
definition syntax within our API, then ask people to write down this
plural definition for their language, then to write every plural form…
all this for custom units which only them will ever see!
Moreover code investigation shows that the singular form was simply
never used, and the plural form was always used (whatever the actual
unit value displayed).
As for the "identifier", this was a text which was never shown anywhere
(except in the unit editor) and for all built-in units, as well as
default unitrc units, it was equivalent to the English plural value.
So we now just have a unique name which is the "long label" to be used
everywhere in the GUI, and abbreviation will be basically the "short
label". That's it. No useless (or worse, not actually usable because it
was not generic internationalization) values anymore!
This fixes all our GObject Introspection issues with GimpUnit which was
both an enum and an int-derived type of user-defined units *completing*
the enum values. GIR clearly didn't like this!
Now GimpUnit is a proper class and units are unique objects, allowing to
compare them with an identity test (i.e. `unit == gimp_unit_pixel ()`
tells us if unit is the pixel unit or not), which makes it easy to use,
just like with int, yet adding also methods, making for nicer
introspected API.
As an aside, this also fixes#10738, by having all the built-in units
retrievable even if libgimpbase had not been properly initialized with
gimp_base_init().
I haven't checked in details how GIR works to introspect, but it looks
like it loads the library to inspect and runs functions, hence
triggering some CRITICALS because virtual methods (supposed to be
initialized with gimp_base_init() run by libgimp) are not set. This new
code won't trigger any critical because the vtable method are now not
necessary, at least for all built-in units.
Note that GimpUnit is still in libgimpbase. It could have been moved to
libgimp in order to avoid any virtual method table (since we need to
keep core and libgimp side's units in sync, PDB is required), but too
many libgimpwidgets widgets were already using GimpUnit. And technically
most of GimpUnit logic doesn't require PDB (only the creation/sync
part). This is one of the reasons why user-created GimpUnit list is
handled and stored differently from other types of objects.
Globally this simplifies the code a lot too and we don't need separate
implementations of various utils for core and libgimp, which means less
prone to errors.
layer_mask points to the original mask created by xcf_load_layer_mask. We copy
this pointer into channel, and xcf_load_channel_props can overwrite this
pointer and free the original mask. If this happens, layer_mask points to
the now-freed original mask, and should not be used.
Therefore we need to change later parts of xcf_load_layer_mask to use channel
instead of layer_mask. Additionally, we add a block and move layer_mask into
this block to guarantee that layer_mask cannot be used after it has
potentially been freed.
Adjustments by Jacob Boerema:
Follow GIMP's code style regarding variables, comment style and
position of braces
See also ASAN output below:
==5247==ERROR: AddressSanitizer: heap-use-after-free on address 0x615000010fd0 at pc 0x7f4e2dbbf31b bp 0x7ffca8a95cd0 sp 0x7ffca8a95cc8
READ of size 8 at 0x615000010fd0 thread T0
#0 0x7f4e2dbbf31a in g_type_check_instance_cast /home/ahunt/git/glib/_build/../gobject/gtype.c:4117:26
#1 0xb200fe in xcf_load_layer_mask /home/ahunt/git/gimp/app/xcf/xcf-load.c:2305:52
#2 0xb18eea in xcf_load_layer /home/ahunt/git/gimp/app/xcf/xcf-load.c:2133:20
#3 0xb13d91 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:499:15
#4 0xb11deb in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
#5 0x619dfd in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:50:17
#6 0x51d364 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 0x506fe2 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 0x50d350 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 0x5373a2 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#10 0x7f4e2c84c349 in __libc_start_main (/lib64/libc.so.6+0x24349)
#11 0x4e0779 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120
0x615000010fd0 is located 336 bytes inside of 504-byte region [0x615000010e80,0x615000011078)
freed by thread T0 here:
#0 0x5e8562 in free /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:127:3
#1 0x7f4e2d76ce08 in g_free /home/ahunt/git/glib/_build/../glib/gmem.c:199:3
#2 0x7f4e2d797a6b in g_slice_free1 /home/ahunt/git/glib/_build/../glib/gslice.c:1183:7
#3 0x7f4e2dbb7b04 in g_type_free_instance /home/ahunt/git/glib/_build/../gobject/gtype.c:2008:5
#4 0x7f4e2db8fe3a in g_object_unref /home/ahunt/git/glib/_build/../gobject/gobject.c:3604:11
#5 0xb22fff in xcf_load_channel_props /home/ahunt/git/gimp/app/xcf/xcf-load.c:1738:13
#6 0xb20037 in xcf_load_layer_mask /home/ahunt/git/gimp/app/xcf/xcf-load.c:2292:9
#7 0xb18eea in xcf_load_layer /home/ahunt/git/gimp/app/xcf/xcf-load.c:2133:20
#8 0xb13d91 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:499:15
#9 0xb11deb in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
#10 0x619dfd in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:50:17
#11 0x51d364 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 0x506fe2 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 0x50d350 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 0x5373a2 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#15 0x7f4e2c84c349 in __libc_start_main (/lib64/libc.so.6+0x24349)
previously allocated by thread T0 here:
#0 0x5e87cd in malloc /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
#1 0x7f4e2d76ccf2 in g_malloc /home/ahunt/git/glib/_build/../glib/gmem.c:106:13
#2 0x7f4e2d7972e0 in g_slice_alloc /home/ahunt/git/glib/_build/../glib/gslice.c:1072:11
#3 0x7f4e2d7978ae in g_slice_alloc0 /home/ahunt/git/glib/_build/../glib/gslice.c:1098:18
#4 0x7f4e2dbb6e0a in g_type_create_instance /home/ahunt/git/glib/_build/../gobject/gtype.c:1911:17
#5 0x7f4e2db9215e in g_object_new_internal /home/ahunt/git/glib/_build/../gobject/gobject.c:1945:24
#6 0x7f4e2db91d1f in g_object_new_valist /home/ahunt/git/glib/_build/../gobject/gobject.c:2288:16
#7 0x7f4e2db90e8b in g_object_new /home/ahunt/git/glib/_build/../gobject/gobject.c:1788:12
#8 0xdb69e0 in gimp_item_new /home/ahunt/git/gimp/app/core/gimpitem.c:723:10
#9 0xce11c8 in gimp_drawable_new /home/ahunt/git/gimp/app/core/gimpdrawable.c:1067:14
#10 0xddf5d8 in gimp_layer_mask_new /home/ahunt/git/gimp/app/core/gimplayermask.c:254:5
#11 0xb1ffc5 in xcf_load_layer_mask /home/ahunt/git/gimp/app/xcf/xcf-load.c:2279:31
#12 0xb18eea in xcf_load_layer /home/ahunt/git/gimp/app/xcf/xcf-load.c:2133:20
#13 0xb13d91 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:499:15
#14 0xb11deb in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
#15 0x619dfd in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:50:17
#16 0x51d364 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 0x506fe2 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 0x50d350 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 0x5373a2 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#20 0x7f4e2c84c349 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:
0x0c2a7fffa1a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffa1b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffa1c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c2a7fffa1d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffa1e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c2a7fffa1f0: fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd
0x0c2a7fffa200: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
0x0c2a7fffa210: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c2a7fffa220: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c2a7fffa230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c2a7fffa240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
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
==5247==ABORTING
./crash-0507799c3e4291570e060f53332b58b8a96f95e5
If a given layer's properties contain multiple PROP_ACTIVE_LAYER entries, we
could add multiple entries for the same layer in info->selected_layers.
However, xcf_load_layer_props and xcf_load_layer both contain logic that
overwrites the layer pointer - they do take care of updating
info->selected_layers and and info->floating_sel (both of which could contain
or point to the current layer). Crucially, they both assume that
info->selected_layers can only contain this layer once - hence they only
remove it from the list once. This logic also frees the old layer because it
thinks that it's no longer in use.
But if we've added the original layer to selected_layers multiple times,
and then use the logic that overwrites layer, selected_layers will still
contain an invalid pointer to the now-freed original layer. Any later
code that tries to read info->selected_layers will hit a user-after-free.
Therefore we need to check if the current layer is already in the
selected_layers list, and avoid adding it again if it's already there.
(It also seems illogical to say that a layer is selected twice, but I'm
focusing on the code-correctness aspect in my analysis.)
Adjustment by Jacob Boerema: the correct way to test that a value does
not exist in a list is to check for the value being negative. Also add
a message to warn users.
ASAN output:
==21241==ERROR: AddressSanitizer: heap-use-after-free on address 0x615000012150 at pc 0x000000d54dbf bp 0x7ffcf813bb60 sp 0x7ffcf813bb58
READ of size 8 at 0x615000012150 thread T0
#0 0xd54dbe in gimp_image_set_selected_layers /home/ahunt/git/gimp/app/core/gimpimage.c:4743:7
#1 0xb146c6 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:690:7
#2 0xb1199b in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
#3 0x619cad in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:35:17
#4 0x51d2e4 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
#5 0x506f62 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
#6 0x50d2d0 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
#7 0x537322 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#8 0x7f064a594349 in __libc_start_main (/lib64/libc.so.6+0x24349)
#9 0x4e06f9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120
0x615000012150 is located 336 bytes inside of 504-byte region [0x615000012000,0x6150000121f8)
freed by thread T0 here:
#0 0x5e84e2 in free /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:127:3
#1 0x7f064b3f81c8 in g_free (/usr/lib64/libglib-2.0.so.0+0x581c8)
#2 0x7f064b41133f in g_slice_free1 (/usr/lib64/libglib-2.0.so.0+0x7133f)
#3 0x7f064b7015c0 in g_type_free_instance /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gtype.c:1946
#4 0xb187d8 in xcf_load_layer /home/ahunt/git/gimp/app/xcf/xcf-load.c:2146:3
#5 0xb13997 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:504:15
#6 0xb1199b in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
#7 0x619cad in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:35:17
#8 0x51d2e4 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
#9 0x506f62 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
#10 0x50d2d0 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
#11 0x537322 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#12 0x7f064a594349 in __libc_start_main (/lib64/libc.so.6+0x24349)
#13 0x4e06f9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120
previously allocated by thread T0 here:
#0 0x5e874d in malloc /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
#1 0x7f064b3f80c8 in g_malloc (/usr/lib64/libglib-2.0.so.0+0x580c8)
#2 0x7f064b410da5 in g_slice_alloc (/usr/lib64/libglib-2.0.so.0+0x70da5)
#3 0x7f064b411258 in g_slice_alloc0 (/usr/lib64/libglib-2.0.so.0+0x71258)
#4 0x7f064b7011d5 in g_type_create_instance /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gtype.c:1849
#5 0x7f064b6e164f in g_object_new_internal /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gobject.c:1827
#6 0x7f064b6e354d in g_object_new_valist /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gobject.c:2152
#7 0x7f064b6e38c8 in g_object_new /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gobject.c:1670
#8 0xdb5b90 in gimp_item_new /home/ahunt/git/gimp/app/core/gimpitem.c:723:10
#9 0xce0358 in gimp_drawable_new /home/ahunt/git/gimp/app/core/gimpdrawable.c:1067:14
#10 0xddd8db in gimp_layer_new /home/ahunt/git/gimp/app/core/gimplayer-new.c:65:11
#11 0xb182a6 in xcf_load_layer /home/ahunt/git/gimp/app/xcf/xcf-load.c:2027:11
#12 0xb13997 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:504:15
#13 0xb1199b in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
#14 0x619cad in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:35:17
#15 0x51d2e4 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
#16 0x506f62 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
#17 0x50d2d0 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
#18 0x537322 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
#19 0x7f064a594349 in __libc_start_main (/lib64/libc.so.6+0x24349)
#20 0x4e06f9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120
SUMMARY: AddressSanitizer: heap-use-after-free /home/ahunt/git/gimp/app/core/gimpimage.c:4743:7 in gimp_image_set_selected_layers
Shadow bytes around the buggy address:
0x0c2a7fffa3d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffa3e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffa3f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c2a7fffa400: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffa410: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c2a7fffa420: fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd
0x0c2a7fffa430: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
0x0c2a7fffa440: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c2a7fffa450: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffa460: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0c2a7fffa470: 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
==21241==ABORTING
( crash-f37f84bd7072e65348ca0e2c920d1e08165356fd )
...to path.
Changes the names of
gimp_vectors_* () API to
gimp_path[s]_* (). Renames related files
to [path] instead of [vectors], along with
relevant enums and functions.
This commit renames the GimpVectors
object to GimpPath in both app/core and
in libgimp. It also renames the files
to gimppath.[ch] and updates the relevant
build and translation files.
There are still outstanding gimp_vectors_* ()
functions on the app side that need to be renamed
in a subsequent commit.
...to paths.
Similar to d0bdbdfd, but covering the
app/core versions of the API instead of
libgimp. Changes the names of
gimp_image_*_vectors () API to
gimp_image_*_path[s] ().
Also renames some related functions such
as gimp_image_pick_path (). The next step
will be to rename the gimp_vectors_* () to
gimp_path_* ().
For now, we can't keep certain GEGL ops
like Gradient as NDE filters, since they are
connected to tools. If the layer is copied
or saved as an .XCF while the tool is still
active, it attempts to save it and causes a
crash.
This patch adds a check to make sure the
copied filter has been commited and that
it's not a tool-based NDE filter before
being copied. It also adds some NULL
checks where gimp_drawable_filter_duplicate
is used to prevent NULL from being
added to the filter stack.
752f0fb4 did not fix saving/loading filters
in .xcf files. To do this, a new function to
retrieve the clip setting was added to
gimpdrawablefilters.c, and then used to
save the value in xcf-save.c. The existing
set_clip function was used in xcf-load.c,
just like in 752f0fb4.
Resolves#11057
Per Idriss Fekir's research, data from GBytes
should be written before the GByte is freed.
Perhaps due to luck, the data persisted on
Windows but was freed too early on Linux.
This patch should resolve the issue.
Otherwise we are not in a valid position anymore and further loading the XCF
fails. This may happen when updating a GEGL operation (in particular removing
effect properties, or renaming them).
In d055564d, a pointer to the color profile
for NDE filter color properties was being
saved rather than the actual ICC profile.
This patch fixes the issue.
It also allows colors to load even if the
color profile is corrupted, with a warning.
We historically have both the colormap and palette concept in core GIMP,
though they are actually kinda the same concept, except that with
"colormap" we work with an array of raw data and it's a lot less
color-aware. It is still much more efficient in some specific cases,
such as when converting the image (we can then convert the whole palette
as a single buffer, because the image palette is space-restricted
anyway), when storing undo data or when storing/loading in XCF.
But for all the rest, let's use gimp_image_get_colormap_palette() and
work with the GimpPalette API.
Certain filters like Supernova have a
random seed property. According to the
specifications it's an unsigned int, so we
can save and load it with existing
functions.
Now that we save GEGL operation arguments ourselves (instead of relying on
GEGL-generated XML), we can serialize and deserialize GimpConfig arguments which
are used in various operations implemented within GIMP core code.
Additionally:
- Also support saving and loading all enum types.
- PROP_EFFECT_ARGUMENT renamed to PROP_FILTER_ARGUMENT for consistent naming.
- A bit more accurate handling on save and load errors with dedicated messages
to various issues.
- Use PROP_FLOAT_OPACITY instead of the obsolete 8-bit PROP_OPACITY (actually
32-bit but stored as 0-255 int).
- Fix a leaking string.
*It's possible to have one layer selected but edit the filter of another layer.
The FilterTool code was updated to use the filter's drawable if editing.
*Prevents editing if multiple layers are selected.
This is a current limitation of the filter tool itself, but it was causing issues with NDE.
*If an XCF with a filter from a third-party plug-in is loaded, the filter won't be
created if the user doesn't have it installed.
Note that GEGL still throws a lot of warnings about missing properties - not sure how to resolve that.
*Refresh drawable when finished editing a filter to fix issue with lingering preview splits
*Fix issue with loading more than one filter after XCF format changes
- Do not leak allocated return value of gegl_node_to_xml_full().
- When merging layer effects, use gimp_drawable_filter_commit(), making
sure we use the exact same code path as when applying layer effects
destructively from the start. This also ensures that filters are
properly removed from the filter stack (unlike
gimp_drawable_merge_filter()), which was the reason why the rendering
was wrong (hence getting the buffer without effects first, then
reapplying it after was only a workaround to an actual bug).
- When removing a filter, verify the object still exists before doing
anything with it. If this was the last reference, we don't want to
call functions on this object. In gimp_drawable_filter_commit(), we
set up a weak pointer. In gimp_drawable_filter_remove_filter() itself,
we save the pointer to the filter's drawable before actual removal (as
we don't want to dereference a freed object later on).
- export_merge_layer_effects() should merge filters recursively through
layer groups.
- clean up the XCF code:
* No need to wrap the effect pointers list into 2 zero offset. Only
have one zero offset to indicate the list end.
* Add the layer effect mask in the effect structure (not in the layer
structure), similar as for layer masks.
* Effect name and icon made as main data in the structure, not as
properties.
This patch implements an initial form of
non-destructive editing. Filters now stay active
instead of being immediately merged down.
A new column is added to the layer tree view, which
can be clicked to show a pop-over menu.
Filters can currently be hidden/shown, edited, reordered,
deleted, and merged down from this pop-over menu.
Currently, this works on layers and layer selections only.
Plenty of room for improvement!
Since now the name of arguments will become more important, over order, let's
name the first parameter "run-mode" even in cases when this is a dummy argument
(most often the case when a procedure always acts the same, whether interactive
or not). I keep the mention of the parameter being useless in the nick and blurb
strings, as it's useful information. But let's keep using our "standard" arg
name "run-mode" for this first argument.
The new parasite format cannot be loaded by old versions of GIMP. This means we
must bump the XCF version (even though technically we didn't really touch the
XCF format itself because text layers are stored in a hackish way, yet text
layers are just too important nowadays to not care).
Nevertheless if an old XCF with text layers was loaded and the text layers left
untouched, the old parasite will be saved back as-is. Therefore no need to bump
the XCF version in this specific case. Only when we created new text layers or
edited existing ones.
xcf_save_prop() starts with va_start(), hence must end with va_end(). Yet any of
the xcf_write*_check_error() macros could end this function abruptly. Therefore
I'm adding a "cleanup" argument to the macros, allowing to add any code
necessary to clean the current function before returning.
These are not used anymore anywhere in our codebase! I'm sure some issues still
exist in various places, yet we can now consider that the multi-item awareness
project is finally over! Wouhou! 🥳
One big question which remains is whether I want to get back to the old naming
of "active" items, rather than "selected" items. The main reason to change the
wording globally was to be able to easily find remnants of non-multi-item aware
code. Now that it's all gone, I could very simply revert to the old naming.
This is in particular a big question for the public API for plug-ins, as the
"active" wording has been used for decades litterally. The only difference now
with how it used to be is that we could have several active items at once.
For the same file, which was saving in ~50 secs in RLE (now ~12), and ~80 secs
in zlib. It now saves zlib-encoded XCF in ~35 secs on the same machine with 8
threads.
Contributed code was not wrong, far from it! But the whole memory management was
a bit on the hard-to-read side. For maintainability, I prefer some simpler code,
which contains a bit more allocation but it's reasonable and at least much less
indexes to play with.
Also instead of gegl_parallel_distribute(), then waiting for each set of batches
to entirely finish, I'm using a GThreadPool which I progressively fill with new
batch data. And finally I'm running the gegl_buffer_get() inside the threads (I
know that we have only reads on the buffer at this point; also GEGL has an
internal lock mechanism anyway).
These actually do not provide a huge additional speedup (compared to the initial
speedup by just going multi-threaded), most likely because the I/O are now the
bottleneck anyway (which is quite a good thing!).
- Coding style fixes.
- Removing gimp_gegl_num_processors unknown variable which didn't exist (coming
from another patch from suzu. See discussion in #4985).
- Removing the `cleanup()` attribute to keep our code as generic as possible
without specific compiler extensions. Just free memory manually as is expected
in C.
Testing on a 115MiB XCF file, saving went down (on my machine, as average on
saving a few times) from about 50 seconds to 15 seconds.
This first version is already pretty neat, though we can probably do even
better.
Instead of storing vectors as properties, they have their own structure, which
make them able to store and load all the usual and common properties of other
items. In other words, it makes XCF now able to store locks, color tags and
several selected paths.
Creates two new parasites to save the black point compensation status
and rendering intent simulation settings in GimpImage.
The parasites are saved and loaded as part of the image in the
.xcf file.
More safety checks for detecting broken xcf files, also based on examining
issue #8230.
After reading an offset where layer, channel, etc. data is stored, we
add a check to make sure that offset is not before where we read the
offset value. Because the data is always written after the offset that
points to it.
Improvements in loading broken xcf files, based on examining issue #8230.
Besides checking for a minimum width and height, GIMP also has a maximum
size we can and should check.
In the case of the image itself, we change invalid dimensions to a size of
1 in hope that the individual layers etc will have the correct size.
For layer, we will also try to go on, but for channel and layer mask, we
will give up.
GIMP could crash if the information regarding old path properties read
from XCF was incorrect. It did not check if xcf_old_path succeeded and
kept trying to load more paths even if the last one failed to load.
Instead we now stop loading paths as soon as that function fails.
In case we have a failure here we also try to skip to the next property
based on the size of the path property, in hopes that the only problem
was this property.
GIMP stopped trying to read the XCF as soon as an invalid parasite was
encountered. However, in this specific case only the parasite data is
invalid, while the rest of the image is not corrupt.
Instead of terminating when we see a corrupt parasite, we skip to the
offset after the parasite. This may still be corrupt, but we can handle
that correctly, see e.g. the XCF in bugzilla issue 685086, which was
the reason of some of the previous changes.
Additionally:
- We add some logging to make it easier to handle future issues in this
area.
- We add tests for a NULL parasite name, and for reading a different
amount of parasite data than we expected. In both cases we return
NULL instead of a parasite.
XCF 17 includes the new visibility locks and the ability to add position
and alpha locks on layer groups.
I am going to push the various commits implementing these different
features together which is why we gather them as a single XCF version.
Due to recent changes on master the linked layers concept doesn't exist
anymore. The conversion code checks whether all linked layers are valid.
However, text layers need special handling.
In the past updating the linked_layers wasn't needed when the layer pointer
of a text layer was changed. Now, that has changed because we parse that
list for valid layers. To fix this we update linked_layers in the same
way as already was done for selected_layers.
Jehan