{"thread":{"id":"51107","subject":"[PATCH 2/2] Use xmmap_gently instead of xmmap in use_pack","startedAt":"2019-05-16T01:49:58Z","lastAt":"2019-05-16T03:34:25Z","messageCount":5,"participants":["Mike Hommey","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"375672","messageId":"20190516003736.25544-2-mh@glandium.org","threadId":"51107","inReplyTo":"20190516003736.25544-1-mh@glandium.org","subject":"[PATCH 2/2] Use xmmap_gently instead of xmmap in use_pack","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-16T00:37:36Z","receivedAt":"2019-05-16T01:49:58Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"use_pack has its own error message on mmap error, but it can't be\nreached when using xmmap, which dies with its own error.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n packfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 16bcb75262..6a66b605e9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -630,7 +630,7 @@ unsigned char *use_pack(struct packed_git *p,\n \t\t\twhile (packed_git_limit < pack_mapped\n \t\t\t\t&& unuse_one_window(p))\n \t\t\t\t; /* nothing */\n-\t\t\twin->base = xmmap(NULL, win->len,\n+\t\t\twin->base = xmmap_gently(NULL, win->len,\n \t\t\t\tPROT_READ, MAP_PRIVATE,\n \t\t\t\tp->pack_fd, win->offset);\n \t\t\tif (win->base == MAP_FAILED)\n-- \n2.21.0.dirty\n\n"},{"id":"375674","messageId":"20190516003736.25544-1-mh@glandium.org","threadId":"51107","inReplyTo":"20190516003130.racexnofe7cxra3t@glandium.org","subject":"[PATCH 1/2] dup() the input fd for fast-import used for remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-16T00:37:35Z","receivedAt":"2019-05-16T01:50:04Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When a remote helper exposes the \"import\" capability, stdout of the\nhelper is sent to stdin of a new fast-import process. This is done by\nsetting the corresponding child_process's in field to the value of the\nout field of the helper child_process.\n\nThe child_process API is defined to close the file descriptors it's\ngiven when calling start_command. This means when start_command is\ncalled for the fast-import process, its input fd (the output fd of the\nhelper), is closed.\n\nBut when the transport helper is later destroyed, in disconnect_helper,\nits input and output are closed, which means close() is called with\nan invalid fd (since it was already closed as per above). Or worse, with\na valid fd owned by something else (since fd numbers can be reused).\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n transport-helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 1f52c95fd8..29787b749e 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -421,7 +421,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti\n \tstruct helper_data *data = transport->data;\n \tint cat_blob_fd, code;\n \tchild_process_init(fastimport);\n-\tfastimport->in = helper->out;\n+\tfastimport->in = xdup(helper->out);\n \targv_array_push(&fastimport->args, \"fast-import\");\n \targv_array_push(&fastimport->args, debug ? \"--stats\" : \"--quiet\");\n \n-- \n2.21.0.dirty\n\n"},{"id":"375677","messageId":"20190516003130.racexnofe7cxra3t@glandium.org","threadId":"51107","inReplyTo":"20190515105609.sucfjvuumeyyrmjb@glandium.org","subject":"Re: Closing fds twice when using remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-16T00:31:30Z","receivedAt":"2019-05-16T01:50:28Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, May 15, 2019 at 07:56:09PM +0900, Mike Hommey wrote:\n> Hi,\n> \n> I started getting a weird error message during some test case involving\n> git-cinnabar, which is a remote-helper to access mercurial\n> repositories.\n> \n> The error says:\n> fatal: mmap failed: Bad file descriptor\n> \n> ... which was not making much sense. Some debugging later, and it turns\n> out this is what happens:\n> \n> - start_command is called for fast-import\n> - start_command is called again for git-remote-hg, passing the\n>   fast_import->out as cmd->in.\n\nMy mistake, it's the opposite: start_command for fast-import is called\nwith the out of git-remote-hg as in.\n\nAnyways, patches incoming.\n\nMike\n"},{"id":"375692","messageId":"20190516032838.GC4596@sigill.intra.peff.net","threadId":"51107","inReplyTo":"20190516003736.25544-1-mh@glandium.org","subject":"Re: [PATCH 1/2] dup() the input fd for fast-import used for remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T03:28:38Z","receivedAt":"2019-05-16T03:28:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 16, 2019 at 09:37:35AM +0900, Mike Hommey wrote:\n\n> When a remote helper exposes the \"import\" capability, stdout of the\n> helper is sent to stdin of a new fast-import process. This is done by\n> setting the corresponding child_process's in field to the value of the\n> out field of the helper child_process.\n> \n> The child_process API is defined to close the file descriptors it's\n> given when calling start_command. This means when start_command is\n> called for the fast-import process, its input fd (the output fd of the\n> helper), is closed.\n> \n> But when the transport helper is later destroyed, in disconnect_helper,\n> its input and output are closed, which means close() is called with\n> an invalid fd (since it was already closed as per above). Or worse, with\n> a valid fd owned by something else (since fd numbers can be reused).\n\nI think this strategy is OK, as explained in my other email.\n\n-Peff\n"},{"id":"375693","messageId":"20190516033422.GD4596@sigill.intra.peff.net","threadId":"51107","inReplyTo":"20190516003736.25544-2-mh@glandium.org","subject":"Re: [PATCH 2/2] Use xmmap_gently instead of xmmap in use_pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T03:34:22Z","receivedAt":"2019-05-16T03:34:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 16, 2019 at 09:37:36AM +0900, Mike Hommey wrote:\n\n> use_pack has its own error message on mmap error, but it can't be\n> reached when using xmmap, which dies with its own error.\n\nMakes sense. Amusingly this xmmap comes from c4712e4553 (Replace mmap\nwith xmmap, better handling MAP_FAILED., 2006-12-24), which specifically\nmoved everything to xmmap for its error handling, and dropped all of the\nMAP_FAILED checks. But it forgot to remove this one.\n\nSo we could also go the other way, and just remove the unreachable\ncheck. I do think the error from use_pack() is better though, so I like\nthe direction you went.\n\n-Peff\n"}]}