{"thread":{"id":"59116","subject":"[PATCH] hash-object: fix descriptor leak with --literally","startedAt":"2023-01-19T01:58:09Z","lastAt":"2023-01-19T08:20:16Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470694","messageId":"Y8ijpJqtkDTi792i@coredump.intra.peff.net","threadId":"59116","inReplyTo":null,"subject":"[PATCH] hash-object: fix descriptor leak with --literally","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-19T01:57:56Z","receivedAt":"2023-01-19T01:58:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In hash_object(), we open a descriptor for each file to hash (whether we\ngot the filename from the command line or --stdin-paths), but never\nclose it. For the traditional code path which feeds the result to\nindex_fd(), this is OK; it closes the descriptor for us.\n\nBut 5ba9a93b39 (hash-object: add --literally option, 2014-09-11) a\nsecond code path which does not close the descriptor. There we need to\ndo so ourselves.\n\nYou can see the problem in a clone of git.git like this:\n\n $ git ls-files -s | grep ^100644 | cut -f2 |\n git hash-object --stdin-paths --literally >/dev/null\n fatal: could not open 'builtin/var.c' for reading: Too many open files\n\nAfter this patch, it completes successfully. I didn't bother with a\ntest, as it's a pain to deal with descriptor limits portably, and the\nfix is so trivial.\n\nSigned-off-by: Jeff King \n---\nSomething I ran into while testing my hash-object fsck series, but I\nbroke it off here because it's really an independent bug-fix.\n\nI do think the world would be less confusing if index_fd() didn't close\nthe descriptor we pass it, and then hash_file() could just do:\n\n fd = open();\n hash_fd(fd);\n close(fd);\n\nwhich is much more readable. But it has many other callers. So even if\nwe wanted to untangle all that, I think it makes sense to do this\nobvious fix in the meantime.\n\n builtin/hash-object.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex b506381502..44db83f07f 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -27,6 +27,7 @@ static int hash_literally(struct object_id *oid, int fd, const char *type, unsig\n \telse\n \t\tret = write_object_file_literally(buf.buf, buf.len, type, oid,\n \t\t\t\t\t\t flags);\n+\tclose(fd);\n \tstrbuf_release(&buf);\n \treturn ret;\n }\n-- \n2.39.1.616.gd06fca9e99\n"},{"id":"470701","messageId":"xmqqtu0n2g67.fsf@gitster.g","threadId":"59116","inReplyTo":"Y8ijpJqtkDTi792i@coredump.intra.peff.net","subject":"Re: [PATCH] hash-object: fix descriptor leak with --literally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-19T06:26:40Z","receivedAt":"2023-01-19T06:26:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In hash_object(), we open a descriptor for each file to hash (whether we\n> got the filename from the command line or --stdin-paths), but never\n> close it. For the traditional code path which feeds the result to\n> index_fd(), this is OK; it closes the descriptor for us.\n>\n> But 5ba9a93b39 (hash-object: add --literally option, 2014-09-11) a\n> second code path which does not close the descriptor.\n\nA sentence without verb? \"5ba9 (hash-...) added a second code path,\nwhich does not close the descriptor.\" or something?\n\n> After this patch, it completes successfully. I didn't bother with a\n> test, as it's a pain to deal with descriptor limits portably, and the\n> fix is so trivial.\n\nTrue. Will queue. Thanks.\n\n> I do think the world would be less confusing if index_fd() didn't close\n> the descriptor we pass it, and then hash_file() could just do:\n>\n> fd = open();\n> hash_fd(fd);\n> close(fd);\n>\n> which is much more readable. But it has many other callers. So even if\n> we wanted to untangle all that, I think it makes sense to do this\n> obvious fix in the meantime.\n\nIndeed, thanks.\n\n> builtin/hash-object.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/hash-object.c b/builtin/hash-object.c\n> index b506381502..44db83f07f 100644\n> --- a/builtin/hash-object.c\n> +++ b/builtin/hash-object.c\n> @@ -27,6 +27,7 @@ static int hash_literally(struct object_id *oid, int fd, const char *type, unsig\n> \telse\n> \t\tret = write_object_file_literally(buf.buf, buf.len, type, oid,\n> \t\t\t\t\t\t flags);\n> +\tclose(fd);\n> \tstrbuf_release(&buf);\n> \treturn ret;\n> }\n"},{"id":"470705","messageId":"Y8j9OP6EeGN6nBDg@coredump.intra.peff.net","threadId":"59116","inReplyTo":"xmqqtu0n2g67.fsf@gitster.g","subject":"Re: [PATCH] hash-object: fix descriptor leak with --literally","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-19T08:20:08Z","receivedAt":"2023-01-19T08:20:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2023 at 10:26:40PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > In hash_object(), we open a descriptor for each file to hash (whether we\n> > got the filename from the command line or --stdin-paths), but never\n> > close it. For the traditional code path which feeds the result to\n> > index_fd(), this is OK; it closes the descriptor for us.\n> >\n> > But 5ba9a93b39 (hash-object: add --literally option, 2014-09-11) a\n> > second code path which does not close the descriptor.\n> \n> A sentence without verb? \"5ba9 (hash-...) added a second code path,\n> which does not close the descriptor.\" or something?\n\nYes, the missing word was \"added\". Thanks.\n\n-Peff\n"}]}