GNOME Bugzilla – Bug 650620
use-after-free in soup cache
Last modified: 2011-06-29 15:41:10 UTC
I have been getting the following warning once in a while with soup when browsing the web on Epiphany: libsoup-CRITICAL **: soup_message_io_pause: assertion `io != NULL' failed I am not sure it's related, but while investigating with valgrind I got the error pasted bellow. The problem seems to be in msg_got_chunk_cb. It fires an asynchronous write using a GString's str pointer, but if a new chunk comes in before the write is actually performed it may happen that g_string_append_len will be called and it might reallocate the str pointer, which is not guaranteed to keep pointing to the same area, and which would cause the original area that was pointed to to be freed, leading to the buffer passed to write() to be invalid. ==24454== Thread 3: ==24454== Syscall param write(buf) points to unaddressable byte(s) ==24454== at 0xC25111D: ??? (syscall-template.S:82) ==24454== by 0x515B6B6: g_local_file_output_stream_write (glocalfileoutputstream.c:192) ==24454== by 0x50F6033: write_async_thread (goutputstream.c:1251) ==24454== by 0x50FB8CB: run_in_thread (gsimpleasyncresult.c:838) ==24454== by 0x50ED2A5: io_job_thread (gioscheduler.c:181) ==24454== by 0x5C913E3: g_thread_pool_thread_proxy (gthreadpool.c:319) ==24454== by 0x5C8ECF5: g_thread_create_proxy (gthread.c:1897) ==24454== by 0xC249B3F: start_thread (pthread_create.c:304) ==24454== by 0xED9B28C: clone (clone.S:112) ==24454== Address 0x262270f0 is 4,224 bytes inside a block of size 16,384 free'd ==24454== at 0x4C27882: realloc (vg_replace_malloc.c:525) ==24454== by 0x5C6E450: g_realloc (gmem.c:233) ==24454== by 0x5C895B6: g_string_maybe_expand (gstring.c:401) ==24454== by 0x5C89C96: g_string_insert_len (gstring.c:741) ==24454== by 0x8DE0588: msg_got_chunk_cb (soup-cache.c:630) ==24454== by 0x53D0E7D: g_closure_invoke (gclosure.c:767) ==24454== by 0x53E28D6: signal_emit_unlocked_R (gsignal.c:3252) ==24454== by 0x53EBD04: g_signal_emit_valist (gsignal.c:2983) ==24454== by 0x53EBED2: g_signal_emit (gsignal.c:3040) ==24454== by 0x4E6530D: read_body_chunk (soup-message-io.c:487) ==24454== by 0x4E65D5F: io_read (soup-message-io.c:958) ==24454== by 0x4E66053: io_unpause_internal (soup-message-io.c:1207)
yeah, so the
oops. so the fix here would be to keep a GSList of SoupBuffers, rather than appending to a GString. (Or possibly, keep a SoupMessageBody, and then append the buffers to that, but I'm not totally sure that would work out right with the SoupMessageBody API.)
Created attachment 189883 [details] [review] Patch Using a GSList to store a list of SoupBuffers
Comment on attachment 189883 [details] [review] Patch >+ guint index = g_slist_length (fixture->buffer_list) - fixture->to_write_index++ - 1; >+ SoupBuffer *buffer = g_slist_nth_data (fixture->buffer_list, index); Eek. You never look at a buffer again after writing it from write_next_buffer(). So it would simplify things a lot if you just removed the buffers from buffer_list as you wrote them. You'd need to keep a pointer to the currently-being-written buffer (so you can free it when it's done being written), but that can just replace the current "writing" flag. And the "has_data_to_write" condition would then just be "fixture->buffer_list != NULL".
(In reply to comment #4) > (From update of attachment 189883 [details] [review]) > >+ guint index = g_slist_length (fixture->buffer_list) - fixture->to_write_index++ - 1; > >+ SoupBuffer *buffer = g_slist_nth_data (fixture->buffer_list, index); > > Eek. > > You never look at a buffer again after writing it from write_next_buffer(). So > it would simplify things a lot if you just removed the buffers from buffer_list > as you wrote them. You'd need to keep a pointer to the currently-being-written > buffer (so you can free it when it's done being written), but that can just > replace the current "writing" flag. And the "has_data_to_write" condition would > then just be "fixture->buffer_list != NULL". Right, I just wanted not to add more fields to the entry struct but it's true that the writing flag can be easily switched to a pointer to the currently writing buffer.
Created attachment 190754 [details] [review] Patch Something like this maybe?
Comment on attachment 190754 [details] [review] Patch looks right. assuming it's tested, go ahead
Committed fdab6aa450d59fdea765c50e8d176f9b963518c9. Thx for the review!