GNOME Bugzilla – Bug 648285
SoupCache eventually corrupts cached files
Last modified: 2011-04-22 07:49:30 UTC
As we use g_file_append_to_async() to create new files in the cache (don't remember exactly why) it could happen (and it indeed does) that the cache ends up appending data to already existing files and thus, corrupting them. The fact that there could be untracked files in the cache directory could happen: * if a file is not completelly written to the disk when we're dumping the cache hash table, then that cache entry will be discarded and thus the file will be created and not tracked. * maybe we are not handling some error case correctly. It could happen that we correctly remove the cache entry but not the file on disk. Anyway, even if the above does not happen, it's more correct to call g_file_create/replace instead of using g_file_append, mainly because we *never* want to append data to an already cached resource.
Created attachment 186344 [details] [review] Patch
Created attachment 186345 [details] [review] Patch v2 It's better to call g_file_create_async first and then g_file_replace_async if the creation fails. That way we save the g_file_query_exists() call (which BTW creates a race condition).
Comment on attachment 186345 [details] [review] Patch v2 > entry->stream = g_object_ref (stream); Hm... this isn't new in this patch, but it looks like that's a leak; it shouldn't be reffing stream. >+ if (entry->error && entry->error->code == G_IO_ERROR_EXISTS) Do if (g_io_error_matches (entry->error, G_IO_ERROR, G_IO_ERROR_EXISTS)) (in particular, "G_IO_ERROR_EXISTS" is just "2", so if you only check ->code and not ->domain, it might actually be some totally different error) However, actually, there's no reason to try create() first and then fall back to replace() if that gives an error; replace() will create the file if it doesn't already exist, so you can just skip the create() step entirely.
(In reply to comment #3) > (From update of attachment 186345 [details] [review]) > However, actually, there's no reason to try create() first and then fall back > to replace() if that gives an error; replace() will create the file if it > doesn't already exist, so you can just skip the create() step entirely. Yeah that's what I thought but when I tried it I was getting also the G_IO_ERROR_EXISTS, will give it another shot...
(In reply to comment #4) > Yeah that's what I thought but when I tried it I was getting also the > G_IO_ERROR_EXISTS, will give it another shot... Were you doing just replace() or just create()? It doesn't make much sense that replace() would return G_IO_ERROR_EXISTS...
Created attachment 186368 [details] [review] Patch v3 Looks like I did something wrong last time I checked as replace() indeed creates a new file if there is none before. BTW good catch (the reference leak).
Comment on attachment 186368 [details] [review] Patch v3 There's going to be a 3.0.1 release next week. If you're confident about this and/or you think it's really important to get into 3.0.1 (and thus F15, opensuse whatever, etc), go ahead and commit. Otherwise wait until post-release, and then it can get more testing before landing on stable
(In reply to comment #7) > (From update of attachment 186368 [details] [review]) > There's going to be a 3.0.1 release next week. If you're confident about this > and/or you think it's really important to get into 3.0.1 (and thus F15, > opensuse whatever, etc), go ahead and commit. Otherwise wait until > post-release, and then it can get more testing before landing on stable I'm reasonably confident about it as there is no logic change in the patch, basically we're just replacing the append by a replace. At least is a must for the WebKitGtk's 1.4.0 as we were easily getting weird corruptions in cache files. I know that at least Xan and Martin (apart from myself) are heavily testing the webkitgtk+stable branch so maybe we could like try it several days and then commit this weekend.
Been testing this for some hours. Can't reproduce anymore the corruption I was seeing, and it also fixes another bug I was seeing that I didn't know was related to the cache. So doubleplusgood so far.
Committed 42f9135da10dffe6d26f1bc6c70fa41fb211b627