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 453365 - gdk_pixbuf_get_file_info crashes on tif files
gdk_pixbuf_get_file_info crashes on tif files
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other Linux
: Normal critical
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2007-07-03 12:26 UTC by Michael Chudobiak
Modified: 2010-07-10 04:05 UTC
See Also:
GNOME target: ---
GNOME version: 2.19/2.20


Attachments
sample crash-causing tif (148.45 KB, image/tiff)
2007-07-03 12:27 UTC, Michael Chudobiak
  Details
backtrace (17.71 KB, text/plain)
2007-07-03 12:28 UTC, Michael Chudobiak
  Details
a patch (691 bytes, patch)
2007-07-03 15:58 UTC, Matthias Clasen
committed Details | Review
fix: width/height set after signal? (587 bytes, patch)
2007-07-03 18:57 UTC, Michael Chudobiak
none Details | Review

Description Michael Chudobiak 2007-07-03 12:26:08 UTC
Calling gdk_pixbuf_get_file_info on any tif file from within gThumb causes a crash on my system. Sample image and backtrace below. (Originally reported in bug 439567). The backtrace suggests that gdk-pixbuf is unhappy with the tif headers. The crash goes away if the patch in bug 403255 is reverted.

- Mike
Comment 1 Michael Chudobiak 2007-07-03 12:27:39 UTC
Created attachment 91095 [details]
sample crash-causing tif
Comment 2 Michael Chudobiak 2007-07-03 12:28:02 UTC
Created attachment 91096 [details]
backtrace
Comment 3 Matthias Clasen 2007-07-03 15:58:52 UTC
Created attachment 91120 [details] [review]
a patch

Hmm, loads just fine here. Anyway, does the attached patch help ?
Comment 4 Michael Chudobiak 2007-07-03 16:24:11 UTC
That stops the crashes, but this warning occurs:

(gthumb:24104): GdkPixbuf-WARNING **: Bug! loader 'tiff' didn't set an error on failure

Background info: gThumb 2.10.4+ checks the height and width of the image using gdk_pixbuf_get_file_info before deciding whether to generate a thumbnail with  gdk_pixbuf_new_from_file_at_scale or with gdk_pixbuf_new_from_file (scaled vs unscaled). (2.10.3 and earlier don't do this check.)

The get_info function seems to choke, but the actual file loader seems OK.

- Mike


Comment 5 Matthias Clasen 2007-07-03 16:28:32 UTC
I'll probably need a small testcase to make further progress on this.
Comment 6 Matthias Clasen 2007-07-03 16:30:37 UTC
2007-07-03  Matthias Clasen  <mclasen@redhat.com>

        * gdk-pixbuf-loader.c (gdk_pixbuf_loader_close): Be
        more careful when calling g_propagate_error().  (#453365,
        Michael Chudobiak)

Comment 7 Michael Chudobiak 2007-07-03 17:31:11 UTC
Partial clue:

io-tiff.c:tiff_image_parse:196 does not set an error message, but all the other checks before and after it do...

- Mike
Comment 8 Matthias Clasen 2007-07-03 18:24:59 UTC
Ok, thanks, I have corrected that.

2007-07-03  Matthias Clasen  <mclasen@redhat.com>

        * io-tiff.c (tiff_image_parse): Always set an error
        when returning NULL.  (453365, Michael Chudobiak)

Comment 9 Michael Chudobiak 2007-07-03 18:57:02 UTC
Created attachment 91133 [details] [review]
fix: width/height set after signal?

Matthias,

Thanks, your patch stops console warnings, but the underlying error is still there.

I think the problem is that gdk_pixbuf_loader_size_func sets the height/width values after emitting a signal, so weird race conditions happen (for instance, my problems went away after inserting debugging printf's...)

A simple patch is attached. I may have misunderstood things, so please make sure it really does make sense...

- Mike
Comment 10 Matthias Clasen 2007-07-04 03:02:04 UTC
Patch doesn't make sense to me. There is no race condition that I can see,
emitting signals is entirely synchronous. What is happening in size_func is
the following:

1. Initialize priv->width and priv->height to the pixbuf dimensions.
We allow to all gdk_pixbuf_loader_set_size() before this point, so 
we only do the initialization if priv->width and priv->height haven't
bee set yet.

        if (priv->width == -1 && priv->height == -1)
                {
                        priv->width = *width;
                        priv->height = *height;
                }

2. Emit the size-prepared signal. Handlers are expected to call gdk_pixbuf_loader_set_size().

        g_signal_emit (loader, pixbuf_loader_signals[SIZE_PREPARED], 0, *width, *height);

3. At this point we no longer accept set_size() calls
 
        priv->size_fixed = TRUE;

4. And we return the values set by the last set_size() call

        *width = priv->width;
        *height = priv->height;


Can you show your size-prepared handler ?
Comment 11 Michael Chudobiak 2007-07-05 13:30:12 UTC
OK, there is no race condition - that was just a theory. But I think I've found the true root cause of the issues. gdk-pixbuf-io.c:info_cb has as its last line:

gdk_pixbuf_loader_set_size (loader, 0, 0);

which is setting the width and height to zero, which persists. Why? Shouldn't it be something like:

gdk_pixbuf_loader_set_size (loader, width, height);

- Mike
Comment 12 Matthias Clasen 2007-07-05 20:03:23 UTC
Oh, hmm, so my last io-tiff.c change was acutally wrong. Here is what the docs say:

<para>
If the function sets @width or @height to zero, the module should interpret
this as a hint that it will be closed soon and shouldn't allocate further
resources. This convention is used to implement gdk_pixbuf_get_file_info()
efficiently.
</para>
Comment 13 Matthias Clasen 2007-07-05 20:17:59 UTC
Let me know if this still doesn't fix things

2007-07-05  Matthias Clasen  <mclasen@redhat.com>

        * io-tiff.c: Revert the last change, it was wrong
        * gdk-pixbuf-loader.c (gdk_pixbuf_loader_close): Redo the last
        change in a different way.

Comment 14 Michael Chudobiak 2007-07-06 11:52:35 UTC
Thanks, it all makes sense now.

I've taken the liberty of committing a comment to tiff_image_parse to explain the early termination signal.

- Mike