GNOME Bugzilla – Bug 667114
Missing check for valid GSocket in g_socket_condition_check()
Last modified: 2012-01-05 11:50:20 UTC
Created attachment 204435 [details] [review] [PATCH] GSocket: Check for a valid socket in g_socket_condition_check g_socket_condition_check() is missing a check for a valid (!= NULL) GSocket. Other functions already have this check. Besides the g_socket_condition_check() function, there are others which are missing the check. Should I submit a patch to add it as well, or should this rather be added as bugs are reported?
Comment on attachment 204435 [details] [review] [PATCH] GSocket: Check for a valid socket in g_socket_condition_check >This patch fixes a bug No, it doesn't. g_return_val_if_fail() means "this is never supposed to happen". It may get rid of a crash in the particular case you're seeing, but the underlying bug is still there and probably messing up other things. (There probably ought to be a g_return_val_if_fail() there, and in another other public APIs that don't currently have one, for consistency, but it doesn't *fix* anything.)
I just had epiphany segfault and tracked it to a crash related to this issue; maybe this will need to be reassigned to libsoup or epiphany. 274 if (!socket->priv->inited) (gdb) bt full
+ Trace 229382
(In reply to comment #1) > (From update of attachment 204435 [details] [review]) > >This patch fixes a bug > > No, it doesn't. g_return_val_if_fail() means "this is never supposed to > happen". It may get rid of a crash in the particular case you're seeing, but > the underlying bug is still there and probably messing up other things. You are right. "fixes a crash" would have been the correct wording. I can change that in a follow-up patch. I was actually hesitating to report this "bug" to GLib because it obviously is caused by libsoup. Then again, I wasn't certain whether what libsoup was doing wasn't a legitimate use of GSocket and if there really is a bug. After reporting this to GLib I was going to write about this to the libsoup mailing list, but then I saw that you were on the Cc list of this bug anyway and didn't. > (There probably ought to be a g_return_val_if_fail() there, and in another > other public APIs that don't currently have one, for consistency, but it > doesn't *fix* anything.) Yes, as I mentioned there are others (I think 3 or 4) that don't have this check. I can add them in a follow-up patch (perhaps along with the rewording you mentioned).
(In reply to comment #2) > I just had epiphany segfault and tracked it to a crash related to this issue; > maybe this will need to be reassigned to libsoup or epiphany. > > 274 if (!socket->priv->inited) > (gdb) bt full That's exactly the same backtrace that I've been seeing. That segfault should be fixed by the patch I posted earlier.
The epiphany crash is 667245. return-if-fails are being added to gsocket in bug 667225. *** This bug has been marked as a duplicate of bug 667245 ***
(In reply to comment #2) > I just had epiphany segfault and tracked it to a crash related to this issue; > maybe this will need to be reassigned to libsoup or epiphany. > > 274 if (!socket->priv->inited) > (gdb) bt full > Fred, could you please follow up in bug 667245?