GNOME Bugzilla – Bug 759341
Avoid endless recursion when copying meta files
Last modified: 2015-12-14 12:51:23 UTC
Created attachment 317194 [details] [review] metadata: Avoid endless recursion when copying meta files meta_builder_copy is traversing the tree while at the same time possibly writing to it. This might leads to endless recursion and consequent segmentation fault. Do the entire copy into a new MetaFile that is not inserted into the source tree and consequently insert the MetaFile once the copy is done. See the downstream bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1285594
Comment on attachment 317194 [details] [review] metadata: Avoid endless recursion when copying meta files For example copy from foo to foo/bar doesn't end up in an endless recursion, however meta_builder_copy is used to implement move, so the original path is removed after copy, so we lose the metadata for foo (also foo/bar) finally... However this should never happen, so it isn't such problem probably. Maybe we want to add some warning for such cases...
Review of attachment 317194 [details] [review]: ::: metadata/metabuilder.c @@ +313,3 @@ + dest->data = temp->data; + dest->children = temp->children; + g_free (temp); This doesn't look right. You're not freeing dest->data and dest->children before overwriting them. There could be something there before, in case meta_builder_lookup didn't create the node. Also, you need to copy last_changed from temp to dest. Calling meta_builder_lookup with dest_path and create=TRUE before the copy_into could create a node that then gets copied when it should not. I think you should call that after you've done the copy_into.
(In reply to Alexander Larsson from comment #2) > Review of attachment 317194 [details] [review] [review]: > > ::: metadata/metabuilder.c > @@ +313,3 @@ > + dest->data = temp->data; > + dest->children = temp->children; > + g_free (temp); > > This doesn't look right. You're not freeing dest->data and dest->children > before overwriting them. There could be something there before, in case > meta_builder_lookup didn't create the node. meta_builder_remove is called at the beginning of meta_builder_copy, which release already existing meta file, so I suppose anything else isn't needed... > Also, you need to copy last_changed from temp to dest. yup, I supposed it is set by the lookup func, it isn't obviously, will fix > Calling meta_builder_lookup with dest_path and create=TRUE before the > copy_into could create a node that then gets copied when it should not. I > think you should call that after you've done the copy_into. dammit, will fix
Created attachment 317342 [details] [review] metadata: Avoid endless recursion when copying meta files
Review of attachment 317342 [details] [review]: looks good to me
Comment on attachment 317342 [details] [review] metadata: Avoid endless recursion when copying meta files Thanks! Pushed to master (commit f839c18), gnome-3-18 (commit 7c20c24) and gnome-3-16 (commit fb99599).