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 691517 - missing check against null in handle_image_selection_changed_cb
missing check against null in handle_image_selection_changed_cb
Status: RESOLVED FIXED
Product: eog
Classification: Core
Component: image viewer
git master
Other Linux
: High critical
: GNOME3.8
Assigned To: EOG Maintainers
EOG Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-01-11 04:09 UTC by landijk-user
Modified: 2013-01-13 17:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backtrace when error occurs (7.53 KB, text/plain)
2013-01-11 04:09 UTC, landijk-user
Details

Description landijk-user 2013-01-11 04:09:12 UTC
Created attachment 233206 [details]
backtrace when error occurs

I just upgraded from Ubuntu P to Q, and I started getting the below error message when closing eog after running it at the command line to view an image.

(eog:17906): EOG-CRITICAL **: eog_list_store_length: assertion `EOG_IS_LIST_STORE (store)' failed

I built from the Ubuntu Q sources and got a backtrace (attached), which indicates that eog_list_store_length is being called by handle_image_selection_changed_cb with null as an argument. Since this call is happening when everything is being cleaned up, I don't think it is an error for "priv->store" to be null, and the bug is that handle_image_selection_changed_cb doesn't check for null before calling eog_list_store_length.

Note that in several other places in the code, there is an explicit check for null before calling the length function, so it appears to be part of the contract for the function that the caller must check for null first. The only question is what we do if priv->store is null.

For reference, this is the block that is executed, conditional on the return value of eog_list_store_length:

	if (eog_list_store_length (EOG_LIST_STORE (priv->store)) == 0) {
		gtk_window_set_title (GTK_WINDOW (window),
				      g_get_application_name());
		gtk_statusbar_remove_all (GTK_STATUSBAR (priv->statusbar),
					  priv->image_info_message_cid);
		eog_scroll_view_set_image (EOG_SCROLL_VIEW (priv->view),
					   NULL);
	}

To preserve the existing behavior, I think the right thing to do is add a guard to guarantee priv->store != null at the beginning of that if statement. Note that if the list is null, the length function returns -1, not 0, so that block doesn't run in the existing code. On the other hand, having a null list is sort of like having an empty list, so maybe that block should run in the null case as well.

Probably the right thing depends on understanding all situations in which we have the null case here, and what is the right thing to do in all those situations. That kind of analysis is beyond my knowledge of this codebase, so I hope someone else can decide what is the right thing. Also, it might be a good idea to check all calls to the length function and determine whether the argument is guaranteed not to be null. I think there might be other instances of this same bug.

I checked your git sources, and it looks to me like this bug was introduced at the below commit  back in 2010, which is a bit odd since I just started observing the behavior. I don't have an explanation for that aspect of this bug, but everything else seems pretty clear.

commit 7a6ea9ee0e5e8a2d89fd16f241a693c5a56f3b78
Author: Felix Riemann <friemann@gnome.org>
Date:   Sat Oct 23 23:54:33 2010 +0200

Are you still around, Felix?
Comment 1 Felix Riemann 2013-01-13 17:02:42 UTC
(In reply to comment #0)
> Are you still around, Felix?

Yes, I am. :)

Nice analysis, thanks for that. Actually priv->store being NULL is a clear sign here that the signal handler shouldn't have been called at all. The actual fix is disconnecting the signal handler when the window is disposed.
---
commit 3145fce6d7544dfb6e159924fb52e2ca13b7a42c
Author: Felix Riemann <p>
Date:   Sun Jan 13 16:58:49 2013 +0100

    EogWindow: Hold a reference on the used ThumbView
    
    The window uses it during runtime so it should keep a reference
    to it. Due to the necessary unref this avoids critical warnings
    because of EogThumbView's own disposal routine causing an
    unwanted signal emission.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=691517
---
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.