GNOME Bugzilla – Bug 680823
g_file_make_directory_with_parents: Fix error propagation
Last modified: 2012-08-16 22:10:38 UTC
When creating a directory fails for some reason other than the parent not existing, don't clear the error before we try to propagate it. To reproduce, run 'ostadmin init /ostree' or otherwise try to run the function on a directory with a parent directory where the current user is not allowed to write.
Created attachment 219879 [details] [review] g_file_make_directory_with_parents: Fix error propagation
This is probably a regression introduced by commit 5a57144d5749efaf3b5e752db9b041597d4b062f Author: Colin Walters <walters@verbum.org> Date: Fri May 4 10:03:12 2012 -0400 gfile: Plug memory leak in g_file_make_directory_with_parents()
Review of attachment 219879 [details] [review]: Makes sense.
Attachment 219879 [details] pushed as b0bce4a - g_file_make_directory_with_parents: Fix error propagation
random applications started segfaulting in g_file_make_directory_with_parents() with the recent GNOME 3.5 updates, it seems that commit might be to blame example, http://redmine.yorba.org/issues/5656 (shotwell didn't change since GNOME 3.4) stacktrace, https://launchpadlibrarian.net/112502489/Stacktrace.txt
Created attachment 221154 [details] [review] g_file_make_directory_with_parents: refix error propagation The patch from b0bce4ad triggered segfaults - see: http://redmine.yorba.org/issues/5656 We were clearing the error before dereferencing it in the next go-around of the while loop - this wasn't necessary.
Review of attachment 221154 [details] [review]: But don't we need to clear the error in the case where !result && my_error->code == G_IO_ERROR_NOT_FOUND at the end of the loop? Actually...I think this code would be a *lot* cleaner if we eliminated the result boolean, and just used my_error != NULL.
(In reply to comment #7) > Review of attachment 221154 [details] [review]: > > But don't we need to clear the error in the case where !result && > my_error->code == G_IO_ERROR_NOT_FOUND at the end of the loop? if '!result && my_error->code == G_IO_ERROR_NOT_FOUND then the loop doesn't end immediately - it contines and the error is cleared. I suppose that then you need to consider what happens if fails: parent_file = g_file_get_parent (work_file); if (parent_file == NULL) break; the code currently would return a FALSE result but no error and the calling code would likely segfault - g_file_get_parent (work_file) means "work file is the root of the file system" - yet doesn't exist... which is certainly pathological but maybe it's better to move the g_clear_error() down a few lines, so the whole function fails with G_IO_ERROR_NOT_FOUND in that case. > Actually...I think this code would be a *lot* cleaner if we eliminated the > result boolean, and just used my_error != NULL. Yeah, perhaps.
Created attachment 221160 [details] [review] g_file_make_directory_with_parents(): fix a corner case If g_file_get_parent() unexpectedly failed, we could return FALSE but with no error.
Created attachment 221161 [details] [review] g_file_make_directory_with_parents: clean up logic Simplify logic by only looking at whether we have a GError and not also using return codes.
Review of attachment 221154 [details] [review]: Ok.
Review of attachment 221160 [details] [review]: This looks safe, yes.
Review of attachment 221161 [details] [review]: ::: gio/gfile.c @@ +3375,3 @@ if (my_error) g_propagate_error (error, my_error); + return my_error != NULL; This should be: return my_error == NULL;
Created attachment 221177 [details] [review] g_file_make_directory_with_parents: clean up logic Simplify logic by only looking at whether we have a GError and not also using return codes.
Created attachment 221178 [details] [review] Add tests for g_file_make_directory_with_parents() Add tests to catch recent regressions with g_file_make_directory_with_parents() This is tested to catch the original two regressions and the bug I tried to introduce with the 'clean up logic' patch.
Review of attachment 221177 [details] [review]: Looks good.
Review of attachment 221178 [details] [review]: ::: gio/tests/live-g-file.c @@ +1046,3 @@ + + g_assert (test_data != NULL); + log ("\n"); Why is this here? @@ +1093,3 @@ + /* No obvious way to do this on Windows */ + if (!posix_compat) + return; Should probably be "goto out;" so that we don't leak memory on Windows either. @@ +1118,3 @@ + g_object_unref (greatgrandchild); + g_object_unref (grandchild); + g_object_unref (child); g_object_unref (root); too
Created attachment 221186 [details] [review] Add tests for g_file_make_directory_with_parents() New version with fixes. The log("\n") was there because I started from a different test which had verbose logging and wanted that to be on a different line than <testname>: - removed.
Review of attachment 221186 [details] [review]: Looks reasonable to me.
The following fixes have been pushed: d7e1d51 Add tests for g_file_make_directory_with_parents() f899358 g_file_make_directory_with_parents: clean up logic 5291190 g_file_make_directory_with_parents(): fix a corner case 732470a g_file_make_directory_with_parents: refix error propagation
Created attachment 221506 [details] [review] Add tests for g_file_make_directory_with_parents() Add tests to catch recent regressions with g_file_make_directory_with_parents()
Created attachment 221507 [details] [review] g_file_make_directory_with_parents: clean up logic Simplify logic by only looking at whether we have a GError and not also using return codes.
Created attachment 221508 [details] [review] g_file_make_directory_with_parents(): fix a corner case If g_file_get_parent() unexpectedly failed, we could return FALSE but with no error.
Created attachment 221509 [details] [review] g_file_make_directory_with_parents: refix error propagation The patch from b0bce4ad triggered segfaults - see: http://redmine.yorba.org/issues/5656 We were clearing the error before dereferencing it in the next go-around of the while loop - this wasn't necessary.