GNOME Bugzilla – Bug 123321
PSD plugin crashes importing a file
Last modified: 2004-12-22 21:47:04 UTC
I have tested the PSD plugin with my ancient collection of PSDs. I found some that cause problems. Attached is a bzip2ed tarball of two PSD files. crash.psd crashes the plugin with the following backtrace: /usr/lib/gimp/1.3/plug-ins/psd: fatal error: Segmentation fault chans_to_RGBA : NULL channel - eep!/usr/lib/gimp/1.3/plug-ins/psd (pid:30980): [E]xit, [H]alt, show [S]tack trace or [P]roceed: s
+ Trace 40463
The other (garbage.psd) opens up with some layers messed up.
Created attachment 20318 [details] psds.tar.bz2
One of the problems (still looking into the details) is that the plug-in tries to add a layer with both width and height equal to zero.
One step further: gimp_layer_new doesn't seem to like names like '... celý clánek' very much. It returns a layer_ID of -1, which results in a later width and height of zero, that makes the gimp_pixel_rgn_init complain. This accounts for 2 layers in the crash.psd picture. The second problem is that actually one of the layers has width and height of zero. This again results in a layer_ID of -1 after a gimp_layer_new, giving the same problems as described above. Problem is pretty clear. Now only a good solution ;)
In general all strings passed to the PDB have to be in UTF-8 encoding. We could add a fallback that attempts to convert from the encoding of the currently active locale to UTF-8. This is what our XCF import already does. See bug 79897.
I'd rather say the PSD plug-in should figure or at least guess what encoding the file has and g_convert() its strings to UTF-8 before passing them to GIMP.
Sure, but I am afraid the PSD file format doesn't specify any encoding, or does it?
I looked in the latest publically availble spec (Photoshop 6.0). It doesn't specify encoding.
Either the PSD plug-in should make use of gimp_any_to_utf8() or the PDB wrapper should be changed to do that for all kinds of names passed to PDB functions. http://developer.gimp.org/api/1.3/libgimpbase/libgimpbase-gimputils.html#gimp-any-to-utf8
Okay, the non-utf8 thing is a gimp2 foible... 1.2.x doesn't care. I'll see if I can conconct a blind-patch.
There's nothing obviously wrong with the loaded garbage.psd in gimp 1.2.x, but then I don't know what it's supposed to look like. It should be a separate bug, I think.
Created attachment 21860 [details] [review] patch for passing utf8 strings
utterly untested attachment 21860 [details] [review] should sort out the utf8 stuff.
The patch doesn't cover the genuinely-0x0-layer problem though. I don't think we can correctly just skip creating these layers, since they might have other data attached to them that shouldn't just be thrown away (at the very least the composition mode, the layer name, etc). GIMP 1.2.x didn't care about making 0x0 layers, it would just not add them to the image's layer list. GIMP 1.3.x does care. The 0x0 layers are (very very likely) a 'real' thing rather than just a mis-parsing of the PSD file. Photoshop usually keeps the saved data shrink-wrapped to the bounds of the pixels that are actually used... I guess it was a bit zealous in this case. Still, it wouldn't be an enormous crime to just skip the 0x0 layers. I think the proper solution is for the PSD plugin to promote a 0x0 layer to a 1x1 layer (with alpha) with a single transparent pixel. Kinda hacky. I just had a stab at doing it that way but I'm not going to have time. I'll see how easy it would be to just skip 0x0 layers instead.
Can someone test the utf8 fix to check for casual regressions? I'll be checking in the utf8 fix soon if there are no objections.
The patch looks good but I wonder what we should do about the 0x0 layers. If 1.2 accepted them, then perhaps 1.3 should allow them too.
The only reason I asked is because the UTF8 thing supposedly a real fix for a real problem, even though it probably doesn't actually completely fix this bug report. That, and I'd like to move the patch out of my local tree so I can move on to some other fixes. :) I don't really know about the 0x0 thing. It was probably only an accident that 1.2 didn't quite choke on 0x0 drawables. It may have been design but I can't easily rationalise it.
Please commit the UTF-8 fix then. That's the best way to get it tested.
I would agree, but without being able to build I can't quite guarantee that this even compiles, though my eyes tell me it's okay. Was hoping to get basic confirmation before potentially breaking the tree.
Well, yes it applies cleanly against CVS and compiles w/o a warning.
Yay. UTF8 stuff now checked-in then.
IIRC Potatoshop makes the user think that all layers are of image size. Or basically all layers are of image size but in order to reduce file size, only the bounding box of all non-transparent pixels is saved. Shouldn't the PSD plug-in create all layers at image size then?
I don't see a compelling reason to do so. We're just trying to load photoshop images, not be a photoshop emulator -- expanding every layer unnecessarily sounds like a recipe for wasting a great deal of memory for no particular reason. (I'm pretty sure that internally photoshop only keeps a sloppy bounding box of the pixels covered and we're getting fairly straight dumps of its [historical] internal structures in the PSD file, because nothing else about the PSD format indicates that the format was designed for efficiency [or quality, extensibility, flexibility, parsability, sanity].) If the GIMP core also wants to offer 'boundless' layers that automatically expand when painted onto, that's its own look-out, and if people go for that paradigm then it's going to be useful regardless of the file format from which the image's data from originally loaded.
First off: I didn't get a crash with crash.psd, the plugin quit itself with "layer blend signature incorrect :-(" instead of a segfault. This makes me think that crash.psd is corrupt. I tried to add a continue statement to the loop that actually adds the layers to the gimpimage. If I could somehow get the plugin to read the image file and crash where it crashed for the reporter I think this would work. I am attaching the patch. This patch just skips the 0x0 layer (since I think this is the more correct thing to do) but it would be fairly trivial to add a fake layer of any size at the same point as the continue.
Created attachment 22814 [details] [review] potential fix for crashing on 0x0 layers. Needs testing for someone who can reproduce the problem
This last patch fixed this assertion for me: (psd:39846): LibGimp-CRITICAL **: file gimppixelrgn.c: line 466 (gimp_pixel_rgn_set_rect): assertion `buf != NULL' failed But it still crashes later. (See ticket 130702).
ok. Two things. First, that second crash should have been reported here, as it is the same bug. The second crash is very confusing because that patch has not been submitted to cvs (things don't get submitted until they are confirmed to work). Second, typing bug #130702 gives us a link to that bug report.
*** Bug 130702 has been marked as a duplicate of this bug. ***
Here is the crash the reporter got after applying the patch. (taken directly from bug #130702) Photoshop import plugin crashes when loading certain photoshop file. Console output: This is a development version of The GIMP. Debug messages may appear here. gimp_composite: use=yes, verbose=no +mmx +sse -sse2 -3dnow -altivec -vis (psd:39846): LibGimp-CRITICAL **: file gimppixelrgn.c: line 466 (gimp_pixel_rgn_set_rect): assertion `buf != NULL' failed (psd:39846): LibGimp-CRITICAL **: file gimppixelrgn.c: line 89 (gimp_pixel_rgn_init): assertion `x >= 0 && x + width <= drawable->width' failed (psd:39846): LibGimp-CRITICAL **: file gimppixelrgn.c: line 467 (gimp_pixel_rgn_set_rect): assertion `x >= 0 && x + width <= pr->drawable->width' failed (psd:39846): LibGimp-CRITICAL **: file gimppixelrgn.c: line 89 (gimp_pixel_rgn_init): assertion `x >= 0 && x + width <= drawable->width' failed /usr/X11R6/libexec/gimp/1.3/plug-ins/psd: fatal error: Segmentation fault chans_to_RGBA : NULL channel - eep!/usr/X11R6/libexec/gimp/1.3/plug-ins/psd (pid:39846): [E]xit, [H]alt, show [S]tack trace or [P]roceed: S
+ Trace 43041
Problematic file is available from http://raven.oook.cz/domenyhome.psd GIMP was compiled with CFLAGS -O -pipe -g, stripped on instalation.
Created attachment 23027 [details] [review] New fix.
This patch incorporates/supersedes the former patches (also removing the 8BIM message change which is a potential crasher). This patch adds the 0 width/height check in (hopefully) all of the remaining places that it matters. It's uncompiled... let us know how it works.
Now it does not segfault, but it don't work either: This is a development version of The GIMP. Debug messages may appear here. gimp_composite: use=yes, verbose=no +mmx +sse -sse2 -3dnow -altivec -vis (psd:87698): LibGimp-CRITICAL **: file gimppixelrgn.c: line 89 (gimp_pixel_rgn_init): assertion `x >= 0 && x + width <= drawable->width' failed (psd:87698): LibGimp-CRITICAL **: file gimppixelrgn.c: line 467 (gimp_pixel_rgn_set_rect): assertion `x >= 0 && x + width <= pr->drawable->width' failed (psd:87698): LibGimp-CRITICAL **: file gimppixelrgn.c: line 89 (gimp_pixel_rgn_init): assertion `x >= 0 && x + width <= drawable->width' failed (gimp-1.3:87695): Gimp-Core-CRITICAL **: file gimpdrawable.c: line 984 (gimp_drawable_data): assertion `GIMP_IS_DRAWABLE (drawable)' failed (gimp-1.3:87695): Gimp-Plug-In-WARNING **: plug-in requested invalid drawable (killing) (gimp-1.3:87695): Gimp-Plug-In-WARNING **: plug_in_close: plug-in aborted before sending its procedure return values
Backtrace of first critical warning: (gdb) bt
+ Trace 43043
There's some stack corruption (second argument in frame #4 is bogus), that drawable is in #5 (gdb) print *drawable $3 = {drawable_id = -1, width = 0, height = 0, bpp = 0, ntile_rows = 0, ntile_cols = 0, tiles = 0x0, shadow_tiles = 0x0} In frame #4 (gdb) print *pr $1 = {data = 0x0, drawable = 0x804f020, bpp = 4, rowstride = 256, x = 0, y = 0, w = 45, h = 10, dirty = 1, shadow = 0, process_count = 134558028} Tell me what else I can check
Created attachment 23035 [details] [review] next version of fix
This one has a minor extra fix or two aimed at this bug, plus the backout of a regressor from another bug. It also turns of verbose debugging. It would help if you could attach the output from running it on the problem file. Thanks.
s/turns of/turns on/
Still don't work, output attached - 3.13-output-release Your patch haven't applied cleanly to 1.3.23 code, so I took psd.c from CVS and that shows regression in handling of layer names, see 3.13-output-CVS (I took single file from CVS, could be caused by that).
Created attachment 23036 [details] console output 3.13-output-release
Created attachment 23037 [details] console output 3.13-output-CVS
In theory that's not a regression in layer names, it's the 'right' thing to do (so I'm told :)).
Created attachment 23041 [details] next next version of fix (whole plugin source)
Please post output with the latest attachment (which isn't a patch). I'm interested that there's a layer claiming to have 0 channels. Wonder if that's a symptom of us losing the plot in that file, or yet another genuine photoshop mysterious shorthand for something.
Created attachment 23042 [details] latest output
now it ends with layer blend signature error
Good, we're getting closer then. I'll follow up when I next have some spare time.
BTW, is domenyhome.psd known to be not-corrupt? i.e. does it at least open in photoshop?
It opens fine with gimp-1.2.5
Yuck. That's a bad sign.
Bad sign? Is more detail available here? Does this spell doom for psd support on the gimp?
No, it simply means that we have a regression, and I hate regressions. Comparing the debug output of the 1.2 and 1.3 plugins would probably pinpoint the divergance quite quickly, but I don't have time at the moment.
Both images now open fine in current CVS.
Confirmed. All files attached/linked here open fine with current CVS.
*** Bug 122602 has been marked as a duplicate of this bug. ***