GNOME Bugzilla – Bug 703461
Crash on destroying a NULL unpause source
Last modified: 2013-08-25 13:10:14 UTC
Patch coming up to add a non-NULL check. I don’t have a copy of the backtrace (and can’t immediately reproduce it), so if this patch looks at all suspect, let me know and I’ll try and come up with reproducer details. This is with libsoup e06d243fadb351b63ba6e7734fd1a9b9fccf4d50.
Created attachment 248224 [details] [review] Fix a crash when destroying a NULL unpause source
Comment on attachment 248224 [details] [review] Fix a crash when destroying a NULL unpause source >- g_source_unref (io->unpause_source); >- io->unpause_source = NULL; >+ if (io->unpause_source) { >+ g_source_unref (io->unpause_source); >+ io->unpause_source = NULL; >+ } hm... make it: g_clear_pointer (&io->unpause_source, g_source_unref); however, it looks like this would only happen if you called soup_session_unpause_message() on a sync session/message, which doesn't work at all anyway, and probably SoupSession should return-if-fail in that case...
(In reply to comment #2) > however, it looks like this would only happen if you called > soup_session_unpause_message() on a sync session/message, which doesn't work at > all anyway, and probably SoupSession should return-if-fail in that case... That may well be what’s happening, although without being able to reproduce it I can’t confirm: https://git.gnome.org/browse/libgdata/tree/gdata/gdata-upload-stream.c#n1059 Unfortunately I can’t test whether things break if I remove the unpause_message() call there, because bug #703464 gets in the way. > hm... make it: > > g_clear_pointer (&io->unpause_source, g_source_unref); In light of the above, do you still want the patch? If so, I’ll make that change and commit.
(In reply to comment #3) > Unfortunately I can’t test whether things break if I remove the > unpause_message() call there, because bug #703464 gets in the way. The unpause_message() call there is 100% wrong, and may even be the cause of bug 703464. > In light of the above, do you still want the patch? If so, I’ll make that > change and commit. shrug. it's still a good cleanup; I fixed a lot places to use the g_clear_* functions a while back, but missed some.
Comment on attachment 248224 [details] [review] Fix a crash when destroying a NULL unpause source Committed with the g_clear_pointer() change. commit 085f4e28907e91c5e5d6517c0205d4288167fd05 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Tue Jul 2 15:00:47 2013 +0100 soup-session: Fix a crash when destroying a NULL unpause source Closes: https://bugzilla.gnome.org/show_bug.cgi?id=703461 libsoup/soup-message-io.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Created attachment 248473 [details] [review] soup-session: Warn if [un]pausing messages from a sync session It was never meant to work, and can lead to weird behaviour and crashes. Also mention this more explicitly in the documentation for soup_session_[un]pause_message(). Helps:
(In reply to comment #4) > (In reply to comment #3) > > Unfortunately I can’t test whether things break if I remove the > > unpause_message() call there, because bug #703464 gets in the way. > > The unpause_message() call there is 100% wrong, and may even be the cause of > bug 703464. It can’t be the cause of bug #703464 because that unpause_message() call is never actually hit before the crash in bug #703464. I’ll remove it anyway though (though won’t commit until I’ve got a better idea of exactly what is wrong). Attachment #248473 [details] is a simple patch to return-if-fail when pausing/unpausing sync messages, plus some documentation additions.
Comment on attachment 248473 [details] [review] soup-session: Warn if [un]pausing messages from a sync session commit 5afc190b6c968a7f06ed5dcb0310384b9840f0fd Author: Philip Withnall <philip@tecnocode.co.uk> Date: Fri Jul 5 19:14:17 2013 +0100 soup-session: Warn if [un]pausing messages from a sync session It was never meant to work, and can lead to weird behaviour and crashes. Also mention this more explicitly in the documentation for soup_session_[un]pause_message(). Helps: https://bugzilla.gnome.org/show_bug.cgi?id=703461 libsoup/soup-session.c | 8 ++++++++ 1 file changed, 8 insertions(+)