{"thread":{"id":"62853","subject":"[PATCH] bundle: avoid closing file descriptor twice","startedAt":"2025-01-25T23:57:39Z","lastAt":"2025-01-26T02:49:58Z","messageCount":2,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511204","messageId":"pull.1857.git.1737849456338.gitgitgadget@gmail.com","threadId":"62853","inReplyTo":null,"subject":"[PATCH] bundle: avoid closing file descriptor twice","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-25T23:57:36Z","receivedAt":"2025-01-25T23:57:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAlready when introduced in c7a8a16239 (Add bundle transport,\n2007-09-10), the `bundle` transport had a bug where it would open a file\ndescriptor to the bundle file and then close it _twice_: First, the file\ndescriptor (`data->fd`) is passed to `unbundle()`, which would use it as\nthe `stdin` of the `index-pack` process, which as a consequence would\nclose it via `start_command()`. However, `data->fd` would still hold the\nnumerical value of the file descriptor, and `close_bundle()` would see\nthat and happily close it again.\n\nThis seems not to have caused too many problems in almost two decades,\nbut I encountered a situation today where it _does_ cause problems: In\ni686 variants of Git for Windows, it seems that file descriptors are\nreused quickly after they have been closed.\n\nIn the particular scenario I faced, `git fetch <bundle> <ref>` gets the\nsame file descriptor value when opening the bundle file and importing\nits embedded packfile (which implicitly closes the file descriptor) and\nthen when opening a pack file in `fetch_and_consume_refs()` while\nlooking up an object's header.\n\nLater on, after the bundle has been imported (and the `close_bundle()`\nfunction erroneously closes the file descriptor that has _already_ been\nclosed when using it as `stdin` for `git index-pack`), the same file\ndescriptor value has now been reused via `use_pack()`. Now, when either\nthe recursive fetch (which defaults to \"on\", unfortunately) or a\ncommit-graph update needs to `mmap()` the packfile, it fails due to a\nnow-invalid file descriptor that _should_ point to the pack file but\ndoesn't anymore.\n\nTo fix that, let's invalidate `data->fd` after calling `unbundle()`.\nThat way, `close_bundle()` does not close a file descriptor that may\nhave been reused for something different. While at it, document that\n`unbundle()` closes the file descriptor, and ensure that it also does\nthat when failing to verify the bundle.\n\nLuckily, this bug does not affect the bundle URI feature, it only\naffects the `git fetch <bundle>` code path.\n\nNote that this patch does not _completely_ clarifies who is responsible\nto close that file descriptor, as `run_command()` may fail _without_\nclosing `cmd->in`. Addressing this issue thoroughly, however, would\nrequire a rather thorough re-design of the `start_command()` and\n`finish_command()` functionality to make it a lot less murky who is\nresponsible for what file descriptors.\n\nAt least this here patch is relatively easy to reason about, and\naddresses a hard failure (`fatal: mmap: could not determine filesize`)\nat the expense of leaking a file descriptor under very rare\ncircumstances in which `git fetch` would error out anyway.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    bundle: avoid closing file descriptor twice\n    \n    This is a really old bug. Deviating from Git's practice, I did not apply\n    this on top of the bugged commit. In fact, I did not even apply this on\n    top of an older maint-* branch because it would cause conflicts even\n    cherry-picking it on top of maint-2.47.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1857%2Fdscho%2Ffix-fetch-bundle-double-close-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1857/dscho/fix-fetch-bundle-double-close-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1857\n\n bundle.c    | 4 +++-\n bundle.h    | 2 ++\n transport.c | 1 +\n 3 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex f18f98fec9c..d7ad6908433 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -607,8 +607,10 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \tif (!opts)\n \t\topts = &opts_fallback;\n \n-\tif (verify_bundle(r, header, opts->flags))\n+\tif (verify_bundle(r, header, opts->flags)) {\n+\t\tclose(bundle_fd);\n \t\treturn -1;\n+\t}\n \n \tstrvec_pushl(&ip.args, \"index-pack\", \"--fix-thin\", \"--stdin\", NULL);\n \ndiff --git a/bundle.h b/bundle.h\nindex a80aa8ad9bf..d664b2f2d61 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -62,6 +62,8 @@ struct unbundle_opts {\n  *\n  * Before unbundling, this method will call verify_bundle() with 'flags'\n  * provided in 'opts'.\n+ *\n+ * Note that the `bundle_fd` will be closed as part of the operation.\n  */\n int unbundle(struct repository *r, struct bundle_header *header,\n \t     int bundle_fd, struct strvec *extra_index_pack_args,\ndiff --git a/transport.c b/transport.c\nindex 10d820c3335..eb808f1e4b7 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -207,6 +207,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \n \tret = unbundle(the_repository, &data->header, data->fd,\n \t\t       &extra_index_pack_args, &opts);\n+\tdata->fd = -1; /* `unbundle()` closes the file descriptor */\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tstrvec_clear(&extra_index_pack_args);\n\nbase-commit: f93ff170b93a1782659637824b25923245ac9dd1\n-- \ngitgitgadget\n"},{"id":"511206","messageId":"xmqqa5bewap7.fsf@gitster.g","threadId":"62853","inReplyTo":"pull.1857.git.1737849456338.gitgitgadget@gmail.com","subject":"Re: [PATCH] bundle: avoid closing file descriptor twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-26T02:49:56Z","receivedAt":"2025-01-26T02:49:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> This seems not to have caused too many problems in almost two decades,\n> but I encountered a situation today where it _does_ cause problems: In\n> i686 variants of Git for Windows, it seems that file descriptors are\n> reused quickly after they have been closed.\n\nNice finding.  It pays not to be in too uniform a monoculture.\n\n>     This is a really old bug. Deviating from Git's practice, I did not apply\n>     this on top of the bugged commit. In fact, I did not even apply this on\n>     top of an older maint-* branch because it would cause conflicts even\n>     cherry-picking it on top of maint-2.47.\n\nJust for fun, attached is my backport to maint-2.44 track.  It\nmerges, with conflict which is rather trivial to resolve, back to\nv2.48.1, on which your original patch is based, and gives the\nmerge result identical to the result of direct application of your\npatch to v2.48.1.\n\nI am not sure what to feel about the FD leak mentioned in the log\nmessage, though.\n\nThanks, will queue.\n\n\n--- >8 ---\nFrom: Johannes Schindelin <johannes.schindelin@gmx.de>\nDate: Sat, 25 Jan 2025 23:57:36 +0000\nSubject: [PATCH] bundle: avoid closing file descriptor twice\n\nAlready when introduced in c7a8a16239 (Add bundle transport,\n2007-09-10), the `bundle` transport had a bug where it would open a file\ndescriptor to the bundle file and then close it _twice_: First, ...\n\n... At least this here patch is relatively easy to reason about, and\naddresses a hard failure (`fatal: mmap: could not determine filesize`)\nat the expense of leaking a file descriptor under very rare\ncircumstances in which `git fetch` would error out anyway.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n bundle.c    | 4 +++-\n bundle.h    | 2 ++\n transport.c | 1 +\n 3 files changed, 6 insertions(+), 1 deletion(-)\n\nObviously this is hard to test ;-)\n\ndiff --git a/bundle.c b/bundle.c\nindex a9744da255..7b3f2da40f 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -616,8 +616,10 @@ int unbundle(struct repository *r, struct bundle_header *header,\n {\n \tstruct child_process ip = CHILD_PROCESS_INIT;\n \n-\tif (verify_bundle(r, header, flags))\n+\tif (verify_bundle(r, header, flags)) {\n+\t\tclose(bundle_fd);\n \t\treturn -1;\n+\t}\n \n \tstrvec_pushl(&ip.args, \"index-pack\", \"--fix-thin\", \"--stdin\", NULL);\n \ndiff --git a/bundle.h b/bundle.h\nindex 021adbdcbb..8b187be7cf 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -50,6 +50,8 @@ int verify_bundle(struct repository *r, struct bundle_header *header,\n  *\n  * Before unbundling, this method will call verify_bundle() with the\n  * given 'flags'.\n+ *\n+ * Note that the `bundle_fd` will be closed as part of the operation.\n  */\n int unbundle(struct repository *r, struct bundle_header *header,\n \t     int bundle_fd, struct strvec *extra_index_pack_args,\ndiff --git a/transport.c b/transport.c\nindex df518ead70..25e2233da0 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -184,6 +184,7 @@ static int fetch_refs_from_bundle(struct transport *transport,\n \t\tget_refs_from_bundle_inner(transport);\n \tret = unbundle(the_repository, &data->header, data->fd,\n \t\t       &extra_index_pack_args, 0);\n+\tdata->fd = -1; /* `unbundle()` closes the file descriptor */\n \ttransport->hash_algo = data->header.hash_algo;\n \treturn ret;\n }\n-- \n2.48.1-293-gbb90fdfe3c\n\n"}]}