GNOME Bugzilla – Bug 561130
get_monitor_geometry segfaults if called with non-existant monitor
Last modified: 2009-11-27 23:42:10 UTC
Steps to reproduce: #! /usr/bin/python import gtk gtk.gdk.screen_get_default().get_monitor_geometry(1) Stack trace: I only get that with gdb, I hope it's a complete stack trace: (gdb) run example.py Starting program: /usr/bin/python example.py [Thread debugging using libthread_db enabled] [New Thread 0x7f029bf536e0 (LWP 9729)] example.py:3: GtkWarning: get_monitor: assertion `monitor_num < screen_x11->n_monitors' failed gtk.gdk.screen_get_default().get_monitor_geometry(1) Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7f029bf536e0 (LWP 9729)] 0x00007f02988f649e in gdk_screen_get_monitor_geometry () from /usr/lib/libgdk-x11-2.0.so.0 Other information: The following should be a C program that does the same thing, and it should tell if this is or isn't a binding problem; however, after an evening spent trying to find the right flags to gcc, I gave up... #include "gdk/gdk.h" #include "gdk/gdkscreen.h" int main() { struct _GdkScreen *s; GdkRectangle r; s= gdk_screen_get_default(); gdk_screen_get_monitor_geometry(s, 0, &r); return 0; }
Bindings are not guilty, this segfaults in GTK+ itself. I don't see how it is critical, you can always call gdk_screen_get_n_monitors() beforehands.
Created attachment 123889 [details] [review] untested fix Quickly written fix for this. I didn't even test if it compiles.
I never thought it is critical. If was just said "a segfault in pygtk is a bug, report it". Thanks Paul.
If anything, this should be g_return_val_if_fail (monitor_num < screen_x11->n_monitors, -1); but since get_monitor() already has that check, it is not exactly urgent to add it here, too. After one of those warnings, all bets are off. What should be done is to improve the docs * @monitor_num: number of the monitor to make it more clear that @monitor_num must be a number between 0 and gdk_screen_get_n_monitors(screen) - 1
(In reply to comment #4) > [...] but since get_monitor() already has that check The problem is that this check in get_monitor() is useless. If the check fails it returns NULL and after all users of that functions segfault by dereferencing NULL.
The purpose of the check is not to prevent a later segfault, but to emit a warning that tells the caller that he called the function with invalid parameters.
(In reply to comment #4) > If anything, this should be > > g_return_val_if_fail (monitor_num < screen_x11->n_monitors, -1); > > but since get_monitor() already has that check, it is not exactly urgent to add > it here, too. After one of those warnings, all bets are off. Imho it is a bug because _get_monitor_geometry does not check screen nor monitor. The fact that there happens to be a warning printed in between doesn't make that lax code any better.
the fact that we print the warning before segfaulting is the most important aspect of adding g_return_if_fail() clauses. If anything is lax here, its the docs.
(In reply to comment #6) > The purpose of the check is not to prevent a later segfault [...] I don't know, maybe I'm a bit skewed since most my latest experience is with higher-level languages with proper exceptions (Java and Python), but for me any segfault is a bug. Especially a segfault in a library, because it is a segfault for every client. And among those GTK+ has binding libraries for languages that don't have segfaulting as a habit. Sure, with C it's kinda hard to avoid segfaults, because client code has full access to everything, but at least GTK+ can avoid segfaults when client code plays "nicely" (i.e. doesn't write to memory where he mustn't etc.). A wrong argument to a function is still "nice" in my understanding.
Yes, every segfault is a bug. In this case, the bug is in the caller...
(In reply to comment #10) > Yes, every segfault is a bug. > In this case, the bug is in the caller... How should for example python know how to fix this? As Paul already noted, many high level languages don't assume segmentation faults as natural result of a faultly function call. Unlike C python can actually be safe in that respect, since scripts cannot randomly play with memory. The only "solution" that doesn't require fixing Gtk would be to add workarounds in all relevant cases in all language bindings. I think it's clear that the non-gtk option would be a tremendous maintenance effort and source of errors.
Not clear what you are hoping to achieve here. GTK+ functions document valid inputs for a reason, and have pretty complete g_return_if_fail checks as a courtesy. That does not mean that it would be in any way acceptable to pass random inputs from python and think thats ok, just because the g_return_if_fail check averts the immediate segfault. If your program segfaults with --g-fatal-warnings, thats still your bug, not GTK+'s.
When I work with Python (or Java, or C#, or ...) I'm expecting that nothing crashes (of course, ideally, but that is at least what is expected). I can get exceptions thrown into my face if I do something wrong, but at least things will not crash. Now, with GTK+, bindings cannot ensure this, at least without additional and unecessary checks that would duplicate all checks GTK+ _already performs_ (except in some cases it prints a warning and segfaults anyway). Thus, from a Python coder perspective, PyGTK feels like an alien that barely wraps some fundamentally unpythonic thing. > Not clear what you are hoping to achieve here. What I'm trying to achieve is that GTK+ is more friendly to bindings. It _is_ easier to make GTK+ not segfault than to force every of 10 binding libraries to perform duplicated checks.
Actually, as a pygtk programmer I would like pygtk to not segfault, but I don't pretend mine to be an enlightened point of view on the advantages and disadvantages of the approach. What I do think is that a common policy regarding this kind of problems should be decided. I just found out another (unrelated) segfault in pygtk (http://poisson.phc.unipi.it/~battiston/segfaulta.py). It's also (probably) quite stupid, but again a segfault is something a python/C#/Java programmer wouldn't expect... ... and I'd like to know if I should report it as a bug or not.
Created attachment 142399 [details] [review] Add more g_return_if_fail guards I think those functions should have g_return_if_fail rather than crashing indirectly in a private function. In fact I think the best appreach would be to move the contents of get_monitor inside the individual functions but I don't know if you would like the code duplication.
Is the entire PyGTK library like this? The last thing I _ever_ want to see in a Python program is a segfault. This is half a step away from stacksmashes due to incorrect pygtk->gtk calls. I don't see how you guys think an assertion failure, _followed_by_a_segfault_ is considered the right way to do it. A workaround in the bindings would be a last resort, but as Paul Pogonyshev said, it's simply redundant in the context of a binding such as pygtk because the assertion should have stopped execution of the pygtk call. Although a workaround in the bindings is still much better than nothing. Adding "--g-fatal-warnings" to the python program just makes it exit instead of segfaulting. Maybe pygtk could raise an exception on failed assertions rather than continuing execution in the gtk library? Or would that cause other issues where a failed assertion is not fatal (if that case even exists)? It's pretty scary seeing that this problem existed since 2008.
I'm for Christian Dywan's patch, although I'm not sure about returning 0 in something such as gdk_screen_get_monitor_height_mm(), then again returning -1 might not be a very good idea either.
> I don't see how you guys think an assertion failure, _followed_by_a_segfault_ > is considered the right way to do it. It is really very simple. If you pass invalid parameter to a function in the C api, you get undefined behaviour, including the possibility of meaningless return values or segfaults. We try to add g_return_if_fail assertions to public entry points _as a convenience_ - triggering one of those still means that your program is invalid and you need to fix the calls that pass invalid parameters into GTK. These assertions are not meant as a hookable exception mechanism for bindings.
They are not meant... but can't they be? I mean: I know there are failed assertions that one could just want to cope with, instead of exiting a program, but - I'm thinking to python, but imagine the story is similar in other bindings - an unwanted exception can be easily catched.
*** Bug 595183 has been marked as a duplicate of this bug. ***
(In reply to comment #18) > If you pass invalid parameter to a function in the C api, you get undefined > behaviour [...] It is unfeasible that binding libraries validate all parameters before passing them onto the main C code. Even if this were coded, it would be enormous duplication of code and effort between the various bindings and even GTK+ itself --- which already does most of the checks. I see only two ways bindings could implement natural-looking (for their target language) argument validation with sane amount of effort. Either GTK+ does it --- with some pluggable mechanism to throw "nice" exceptions for bindings or not, --- or argument semantics is added to GObject introspection.
I've made all the changes that I'm willing to make here now: commit 43a07af9b131929d26eac8259c15f1e7bf0ddf1e Author: Matthias Clasen <mclasen@redhat.com> Date: Fri Nov 27 18:39:15 2009 -0500 Be a bit more forgiving about invalid monitor number By moving the g_return_if_fail() checks into individual monitor functions. See bug 561130.