{"thread":{"id":"7802","subject":"[PATCH] Fix handle leak in write_tree","startedAt":"2007-04-23T19:49:25Z","lastAt":"2007-04-25T22:28:17Z","messageCount":7,"participants":["Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"40233","messageId":"20070423194925.GA5163@steel.home","threadId":"7802","inReplyTo":null,"subject":"[PATCH] Fix handle leak in write_tree","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-23T19:49:25Z","receivedAt":"2007-04-23T19:49:25Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"This is a quick and dirty fix for the broken \"git cherry-pick -n\" on\nsome broken OS, which does not remove the directory entry after unlink\nsucceeded(!) if the file is still open somewher.\nThe entry is left but \"protected\": no open, no unlink, no stat.\nVery annoying.\n\nSigned-off-by: Alex Riesen \n---\n\nThat should be enough to get going, but I have to say that the\ninterface to lockfiles is really troublesome. Why has the caller close\na handle it didn't open? Especially if there are perfect matches for\nthe opening function (hold_locked_index) in form of commit and\nrollback?\n\nHow about something like this (just interface):\n\nstruct lock_file\n{\n\tstruct lock_file *next;\n\tpid_t owner;\n\tint fd;\n\tchar on_list;\n\tchar filename[PATH_MAX];\n};\n\nstruct lock_file *open_locked(const char *path, int die_on_error);\nstruct lock_file *open_index_locked(int die_on_error);\nvoid commit_lock_file(struct lock_file *); /* always assuming .lock */\nvoid rollback_lock_file(struct lock_file *);\n\n builtin-write-tree.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-write-tree.c b/builtin-write-tree.c\nindex c88bbd1..d8284b4 100644\n--- a/builtin-write-tree.c\n+++ b/builtin-write-tree.c\n@@ -29,6 +29,7 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)\n \n \twas_valid = cache_tree_fully_valid(active_cache_tree);\n \n+\tclose(newfd);\n \tif (!was_valid) {\n \t\tif (cache_tree_update(active_cache_tree,\n \t\t\t\t active_cache, active_nr,\n@@ -36,8 +37,10 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)\n \t\t\tdie(\"git-write-tree: error building trees\");\n \t\tif (0 <= newfd) {\n \t\t\tif (!write_cache(newfd, active_cache, active_nr)\n-\t\t\t\t\t&& !close(newfd))\n+\t\t\t\t\t&& !close(newfd)) {\n \t\t\t\tcommit_lock_file(lock_file);\n+\t\t\t\tnewfd = -1;\n+\t\t\t}\n \t\t}\n \t\t/* Not being able to write is fine -- we are only interested\n \t\t * in updating the cache-tree part, and if the next caller\n@@ -55,6 +58,8 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)\n \telse\n \t\thashcpy(sha1, active_cache_tree->sha1);\n \n+\tif (0 <= newfd)\n+\t\tclose(newfd);\n \trollback_lock_file(lock_file);\n \n \treturn 0;\n-- \n1.5.1.1.946.gdb75a\n"},{"id":"40288","messageId":"7vps5u9wsk.fsf@assigned-by-dhcp.cox.net","threadId":"7802","inReplyTo":"20070423194925.GA5163@steel.home","subject":"Re: [PATCH] Fix handle leak in write_tree","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-24T05:16:43Z","receivedAt":"2007-04-24T05:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> This is a quick and dirty fix for the broken \"git cherry-pick -n\" on\n> some broken OS, which does not remove the directory entry after unlink\n> succeeded(!) if the file is still open somewher.\n> The entry is left but \"protected\": no open, no unlink, no stat.\n> Very annoying.\n>\n> Signed-off-by: Alex Riesen \n> ---\n>\n> That should be enough to get going, but I have to say that the\n> interface to lockfiles is really troublesome. Why has the caller close\n> a handle it didn't open? Especially if there are perfect matches for\n> the opening function (hold_locked_index) in form of commit and\n> rollback?\n>\n> How about something like this (just interface):\n>\n> struct lock_file\n> {\n> \tstruct lock_file *next;\n> \tpid_t owner;\n> \tint fd;\n> \tchar on_list;\n> \tchar filename[PATH_MAX];\n> };\n>\n> struct lock_file *open_locked(const char *path, int die_on_error);\n> struct lock_file *open_index_locked(int die_on_error);\n> void commit_lock_file(struct lock_file *); /* always assuming .lock */\n> void rollback_lock_file(struct lock_file *);\n\nI agree that making commit and rollback close the file\ndescriptor and lock holders to use lock->fd for write() makes\nmore sense, although it is a bit unclear from the above set of\nfunction signatures what your plan on the lifetime rule for\n\"struct lock_file\" is. If it will be linked to the list given\nto the atexit() handler and the caller of open_locked() never\nfrees it, I think I am fine with the interface.\n"},{"id":"40312","messageId":"81b0412b0704240230x3a5b473k5da3d45d9c997c3b@mail.gmail.com","threadId":"7802","inReplyTo":"7vps5u9wsk.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix handle leak in write_tree","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-24T09:30:07Z","receivedAt":"2007-04-24T09:30:07Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 4/24/07, Junio C Hamano wrote:\n> > How about something like this (just interface):\n> >\n> > struct lock_file\n> > {\n> > struct lock_file *next;\n> > pid_t owner;\n> > int fd;\n> > char on_list;\n> > char filename[PATH_MAX];\n> > };\n> >\n> > struct lock_file *open_locked(const char *path, int die_on_error);\n> > struct lock_file *open_index_locked(int die_on_error);\n> > void commit_lock_file(struct lock_file *); /* always assuming .lock */\n> > void rollback_lock_file(struct lock_file *);\n>\n> I agree that making commit and rollback close the file\n> descriptor and lock holders to use lock->fd for write() makes\n> more sense, although it is a bit unclear from the above set of\n> function signatures what your plan on the lifetime rule for\n> \"struct lock_file\" is. If it will be linked to the list given\n> to the atexit() handler and the caller of open_locked() never\n> frees it, I think I am fine with the interface.\n\nI actually expected the caller to define the lifetime. atexit is not exactly\nlibification-effort friendly. I could imagine open*locked with an additional\nargument for atexit registration, I just don't like the idea (dislike\ndie_on_error\ntoo). Isn't such kind of resource control _generally_ nicer to implement\nin the top levels of a program?\n"},{"id":"40313","messageId":"7vps5u86bn.fsf@assigned-by-dhcp.cox.net","threadId":"7802","inReplyTo":"81b0412b0704240230x3a5b473k5da3d45d9c997c3b@mail.gmail.com","subject":"Re: [PATCH] Fix handle leak in write_tree","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-24T09:33:48Z","receivedAt":"2007-04-24T09:33:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> ... Isn't such kind of resource control _generally_ nicer to implement\n> in the top levels of a program?\n\nIn theory perhaps, but my understanding of our use of atexit()\nis to clean them up in situations beyond the control of the top\nlevels of a program, most notably upon exit on signal.\n"},{"id":"40450","messageId":"20070425205930.GF30061@steel.home","threadId":"7802","inReplyTo":"7vps5u86bn.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix handle leak in write_tree","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-25T20:59:30Z","receivedAt":"2007-04-25T20:59:30Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Tue, Apr 24, 2007 11:33:48 +0200:\n> \n> > ... Isn't such kind of resource control _generally_ nicer to implement\n> > in the top levels of a program?\n> \n> In theory perhaps, but my understanding of our use of atexit()\n> is to clean them up in situations beyond the control of the top\n> levels of a program, most notably upon exit on signal.\n> \n\nstruct git_context;\n/* or whatever is the latest name for the repo bookeeping is.\n It have to be passed to every git-related function anyway. */\n\nstruct lock_file *open_index_locked(struct git_context *, int die_on_error);\nint commit_index_locked(struct git_context *, struct lock_file *);\n... and so on.\n\nThen let the top level call something like\n\n git_cleanup(struct git_context *);\n\nin its _own_ signal or atexit handlers. If it didn't setup the\nhandlers, than perhaps it did want it so (leaving tempfiles behind is\nsometimes done on purpose). Or it _is_ a bug, but then it is clear:\nyou have to cleanup, and you do git's part of cleanup with\ngit_cleanup.\n"},{"id":"40458","messageId":"7vslaoupbp.fsf@assigned-by-dhcp.cox.net","threadId":"7802","inReplyTo":"20070425205930.GF30061@steel.home","subject":"Re: [PATCH] Fix handle leak in write_tree","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-25T21:16:58Z","receivedAt":"2007-04-25T21:16:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Then let the top level call something like\n>\n> git_cleanup(struct git_context *);\n>\n> in its _own_ signal or atexit handlers. If it didn't setup the\n> handlers, than perhaps it did want it so (leaving tempfiles behind is\n> sometimes done on purpose). Or it _is_ a bug, but then it is clear:\n> you have to cleanup, and you do git's part of cleanup with\n> git_cleanup.\n\nI would agree that libification would need to go something along\nthat line.\n\nIn the meantime for 1.5.2 stabilization, let's do the minimum\nfix of applying your earlier patch.\n"},{"id":"40464","messageId":"20070425222817.GI30061@steel.home","threadId":"7802","inReplyTo":"20070423194925.GA5163@steel.home","subject":"[PATCH] Fix handle leak in write_tree","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-04-25T22:28:17Z","receivedAt":"2007-04-25T22:28:17Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"This is a quick and dirty fix for the broken \"git cherry-pick -n\" on\nsome broken OS, which does not remove the directory entry after unlink\nsucceeded(!) if the file is still open somewher.\nThe entry is left but \"protected\": no open, no unlink, no stat.\nVery annoying.\n\nSigned-off-by: Alex Riesen \n---\nAlex Riesen, Mon, Apr 23, 2007 21:49:25 +0200:\n> diff --git a/builtin-write-tree.c b/builtin-write-tree.c\n> index c88bbd1..d8284b4 100644\n> --- a/builtin-write-tree.c\n> +++ b/builtin-write-tree.c\n> @@ -29,6 +29,7 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)\n> \n> \twas_valid = cache_tree_fully_valid(active_cache_tree);\n> \n> +\tclose(newfd);\n> \tif (!was_valid) {\n> \t\tif (cache_tree_update(active_cache_tree,\n> \t\t\t\t active_cache, active_nr,\n\nBTW, can someone explain this hunk? And how does it manage to\npass the tests?! And HOW did it manage to pass me?!!!\n\n builtin-write-tree.c | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-write-tree.c b/builtin-write-tree.c\nindex c88bbd1..391de53 100644\n--- a/builtin-write-tree.c\n+++ b/builtin-write-tree.c\n@@ -36,8 +36,10 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)\n \t\t\tdie(\"git-write-tree: error building trees\");\n \t\tif (0 <= newfd) {\n \t\t\tif (!write_cache(newfd, active_cache, active_nr)\n-\t\t\t\t\t&& !close(newfd))\n+\t\t\t\t\t&& !close(newfd)) {\n \t\t\t\tcommit_lock_file(lock_file);\n+\t\t\t\tnewfd = -1;\n+\t\t\t}\n \t\t}\n \t\t/* Not being able to write is fine -- we are only interested\n \t\t * in updating the cache-tree part, and if the next caller\n@@ -55,6 +57,8 @@ int write_tree(unsigned char *sha1, int missing_ok, const char *prefix)\n \telse\n \t\thashcpy(sha1, active_cache_tree->sha1);\n \n+\tif (0 <= newfd)\n+\t\tclose(newfd);\n \trollback_lock_file(lock_file);\n \n \treturn 0;\n-- \n1.5.2.rc0.65.gd296e\n"}]}