{"thread":{"id":"51099","subject":"Closing fds twice when using remote helpers","startedAt":"2019-05-15T10:56:14Z","lastAt":"2019-05-16T22:02:17Z","messageCount":10,"participants":["Mike Hommey","Ævar Arnfjörð Bjarmason","Johannes Sixt","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"375621","messageId":"20190515105609.sucfjvuumeyyrmjb@glandium.org","threadId":"51099","inReplyTo":null,"subject":"Closing fds twice when using remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-15T10:56:09Z","receivedAt":"2019-05-15T10:56:14Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nI started getting a weird error message during some test case involving\ngit-cinnabar, which is a remote-helper to access mercurial\nrepositories.\n\nThe error says:\nfatal: mmap failed: Bad file descriptor\n\n... which was not making much sense. Some debugging later, and it turns\nout 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- in start_command, we end up on the line of code that does\n  close(cmd->in), so fast_import->out/cmd->in is now closed\n- much later, in disconnect_helper, we call close(data->helper->out),\n  where data->helper is the cmd for fast-import, and that fd was already\nclosed above.\n- Except, well, fds being what they are, we in fact just closed a fd\n  from a packed_git->pack_fd. So, when use_pack is later called, and\n  tries to mmap data from that pack, it fails because the file\n  descriptor was closed.\n\nI'm not entirely sure how to address this... Any ideas?\n\nRelatedly, use_pack calls xmmap, which does its own error handling and\ndie()s in case of error, but then goes on to do its own check with a\ndifferent error message (which, in fact, could be more useful in other\ncases). It seems like it should call xmmap_gently instead.\n\nCheers,\n\nMike\n"},{"id":"375622","messageId":"87bm04vt81.fsf@evledraar.gmail.com","threadId":"51099","inReplyTo":"20190515105609.sucfjvuumeyyrmjb@glandium.org","subject":"Re: Closing fds twice when using remote helpers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-15T11:43:10Z","receivedAt":"2019-05-15T11:43:16Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 15 2019, Mike Hommey wrote:\n\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> - in start_command, we end up on the line of code that does\n>   close(cmd->in), so fast_import->out/cmd->in is now closed\n> - much later, in disconnect_helper, we call close(data->helper->out),\n>   where data->helper is the cmd for fast-import, and that fd was already\n> closed above.\n> - Except, well, fds being what they are, we in fact just closed a fd\n>   from a packed_git->pack_fd. So, when use_pack is later called, and\n>   tries to mmap data from that pack, it fails because the file\n>   descriptor was closed.\n>\n> I'm not entirely sure how to address this... Any ideas?\n>\n> Relatedly, use_pack calls xmmap, which does its own error handling and\n> die()s in case of error, but then goes on to do its own check with a\n> different error message (which, in fact, could be more useful in other\n> cases). It seems like it should call xmmap_gently instead.\n\nThe \"obvious\" hacky fix is to pass in some \"I own it, don't close it\"\nnew flag in the child_process struct.\n\nIn fact we used to have such a thing in the code, see e72ae28895\n(\"start_command(), .in/.out/.err = -1: Callers must close the file\ndescriptor\", 2008-02-16).\n\nSo we could bring it back, but I wonder if a better long-term solution\nis to refactor the API to have explicit start_command() ->\nfree_command() steps, even if the free() is something that happens\nimplicitly unless some \"gutsy\" function is called.\n"},{"id":"375635","messageId":"b6ff2486-a1c2-4e89-4338-9e4e56d528bc@kdbg.org","threadId":"51099","inReplyTo":"87bm04vt81.fsf@evledraar.gmail.com","subject":"Re: Closing fds twice when using remote helpers","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-05-15T17:59:49Z","receivedAt":"2019-05-15T17:59:53Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.05.19 um 13:43 schrieb Ævar Arnfjörð Bjarmason:\n> \n> On Wed, May 15 2019, Mike Hommey wrote:\n> \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\nI guess, you request fast_import->out = -1.\n\n>> - start_command is called again for git-remote-hg, passing the\n>>   fast_import->out as cmd->in.\n\nOK.\n\n>> - in start_command, we end up on the line of code that does\n>>   close(cmd->in), so fast_import->out/cmd->in is now closed\n\nYes. That's how the interface is specified.\n\n>> - much later, in disconnect_helper, we call close(data->helper->out),\n>>   where data->helper is the cmd for fast-import, and that fd was already\n>> closed above.\n\nThat must is wrong. Passing a fd to start_command() relinquishes\nresponsibility.\n\n>> - Except, well, fds being what they are, we in fact just closed a fd\n>>   from a packed_git->pack_fd. So, when use_pack is later called, and\n>>   tries to mmap data from that pack, it fails because the file\n>>   descriptor was closed.\n\nEither dup() the file descriptor, or mmap() before you call the\nconsuming start_command().\n\n>> I'm not entirely sure how to address this... Any ideas?\n>>\n>> Relatedly, use_pack calls xmmap, which does its own error handling and\n>> die()s in case of error, but then goes on to do its own check with a\n>> different error message (which, in fact, could be more useful in other\n>> cases). It seems like it should call xmmap_gently instead.\n> \n> The \"obvious\" hacky fix is to pass in some \"I own it, don't close it\"\n> new flag in the child_process struct.\n> \n> In fact we used to have such a thing in the code, see e72ae28895\n> (\"start_command(), .in/.out/.err = -1: Callers must close the file\n> descriptor\", 2008-02-16).\n\nThat's a different thing. -1 tells that a pipe end should be passed back\nto the caller. Of course, it must not be closed by start_command.\n\nBut if the caller passes their own fd *into* start_command/run_command,\nthen the caller must not close it.\n\n> So we could bring it back, but I wonder if a better long-term solution\n> is to refactor the API to have explicit start_command() ->\n> free_command() steps, even if the free() is something that happens\n> implicitly unless some \"gutsy\" function is called.\n\n*Shrug* I'd use C++ to make the interface a no-brainer.\n\n-- Hannes\n"},{"id":"375658","messageId":"20190515220834.svurujspjt3bmath@glandium.org","threadId":"51099","inReplyTo":"b6ff2486-a1c2-4e89-4338-9e4e56d528bc@kdbg.org","subject":"Re: Closing fds twice when using remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-15T22:08:34Z","receivedAt":"2019-05-15T22:08:44Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, May 15, 2019 at 07:59:49PM +0200, Johannes Sixt wrote:\n> Am 15.05.19 um 13:43 schrieb Ævar Arnfjörð Bjarmason:\n> > \n> > On Wed, May 15 2019, Mike Hommey wrote:\n> > \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> \n> I guess, you request fast_import->out = -1.\n> \n> >> - start_command is called again for git-remote-hg, passing the\n> >>   fast_import->out as cmd->in.\n> \n> OK.\n> \n> >> - in start_command, we end up on the line of code that does\n> >>   close(cmd->in), so fast_import->out/cmd->in is now closed\n> \n> Yes. That's how the interface is specified.\n> \n> >> - much later, in disconnect_helper, we call close(data->helper->out),\n> >>   where data->helper is the cmd for fast-import, and that fd was already\n> >> closed above.\n> \n> That must is wrong. Passing a fd to start_command() relinquishes\n> responsibility.\n> \n> >> - Except, well, fds being what they are, we in fact just closed a fd\n> >>   from a packed_git->pack_fd. So, when use_pack is later called, and\n> >>   tries to mmap data from that pack, it fails because the file\n> >>   descriptor was closed.\n> \n> Either dup() the file descriptor, or mmap() before you call the\n> consuming start_command().\n\nYou seem to imply this is my code doing something. It's not. The process\nthat is doing all the things I noted above is an unmodified `git fetch`,\nwhen using a remote-helper transport. The use_pack happens after the\ntransport is disposed because that's at the end of git fetch, when it\nupdates refs.\n\nAnyway, it would seem the fix is to dup(out) when passing it as in to\nstart_command?\n\nMike\n"},{"id":"375673","messageId":"20190516004802.fmtdz52xxcplkgcf@glandium.org","threadId":"51099","inReplyTo":"20190515235339.GA3579@sigill.intra.peff.net","subject":"Re: Closing fds twice when using remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-16T00:48:02Z","receivedAt":"2019-05-16T01:50:03Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, May 15, 2019 at 07:53:40PM -0400, Jeff King wrote:\n> On Thu, May 16, 2019 at 07:08:34AM +0900, Mike Hommey wrote:\n> \n> > > >> - Except, well, fds being what they are, we in fact just closed a fd\n> > > >>   from a packed_git->pack_fd. So, when use_pack is later called, and\n> > > >>   tries to mmap data from that pack, it fails because the file\n> > > >>   descriptor was closed.\n> > > \n> > > Either dup() the file descriptor, or mmap() before you call the\n> > > consuming start_command().\n> \n> If I understand your case correctly, the mmap() is not relevant.  The\n> issue is that we close a random file descriptor accidentally, and it\n> just happens to be a pack descriptor later on. Right?\n> \n> > You seem to imply this is my code doing something. It's not. The process\n> > that is doing all the things I noted above is an unmodified `git fetch`,\n> > when using a remote-helper transport. The use_pack happens after the\n> > transport is disposed because that's at the end of git fetch, when it\n> > updates refs.\n> \n> Yes, it's a bug in git.\n> \n> > Anyway, it would seem the fix is to dup(out) when passing it as in to\n> > start_command?\n> \n> Generally, yes. It looks like maybe this spot?\n> \n> diff --git a/transport-helper.c b/transport-helper.c\n> index fcd2a58d0e..45cdf891ec 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -433,7 +433,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> \n> One thing I'd wonder, though: what is the contract between the helper\n> and fast-import here? In the current code, when the helper closes its\n> stdout, fast-import will see EOF. But not if we are holding on to\n> another copy of the descriptor.\n\nThe helper is supposed to finish the fast-import stream with \"done\".\nThe documentation doesn't say much, but it also seems like the helper\ncould theoretically continue to respond to commands it's sent after\nhaving done so, but that currently never happens AFAICT.\n\nMike\n"},{"id":"375679","messageId":"20190515235339.GA3579@sigill.intra.peff.net","threadId":"51099","inReplyTo":"20190515220834.svurujspjt3bmath@glandium.org","subject":"Re: Closing fds twice when using remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-15T23:53:40Z","receivedAt":"2019-05-16T01:50:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 16, 2019 at 07:08:34AM +0900, Mike Hommey wrote:\n\n> > >> - Except, well, fds being what they are, we in fact just closed a fd\n> > >>   from a packed_git->pack_fd. So, when use_pack is later called, and\n> > >>   tries to mmap data from that pack, it fails because the file\n> > >>   descriptor was closed.\n> > \n> > Either dup() the file descriptor, or mmap() before you call the\n> > consuming start_command().\n\nIf I understand your case correctly, the mmap() is not relevant.  The\nissue is that we close a random file descriptor accidentally, and it\njust happens to be a pack descriptor later on. Right?\n\n> You seem to imply this is my code doing something. It's not. The process\n> that is doing all the things I noted above is an unmodified `git fetch`,\n> when using a remote-helper transport. The use_pack happens after the\n> transport is disposed because that's at the end of git fetch, when it\n> updates refs.\n\nYes, it's a bug in git.\n\n> Anyway, it would seem the fix is to dup(out) when passing it as in to\n> start_command?\n\nGenerally, yes. It looks like maybe this spot?\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex fcd2a58d0e..45cdf891ec 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -433,7 +433,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\nOne thing I'd wonder, though: what is the contract between the helper\nand fast-import here? In the current code, when the helper closes its\nstdout, fast-import will see EOF. But not if we are holding on to\nanother copy of the descriptor.\n\nIn that case, the right solution is probably more like:\n\n  fastimport->in = helper->out;\n  helper->out = -1; /* hand ownership to fast-import */\n\n-Peff\n"},{"id":"375691","messageId":"20190516032802.GB4596@sigill.intra.peff.net","threadId":"51099","inReplyTo":"20190516004802.fmtdz52xxcplkgcf@glandium.org","subject":"Re: Closing fds twice when using remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T03:28:02Z","receivedAt":"2019-05-16T03:28:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 16, 2019 at 09:48:02AM +0900, Mike Hommey wrote:\n\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > index fcd2a58d0e..45cdf891ec 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -433,7 +433,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> > \n> > One thing I'd wonder, though: what is the contract between the helper\n> > and fast-import here? In the current code, when the helper closes its\n> > stdout, fast-import will see EOF. But not if we are holding on to\n> > another copy of the descriptor.\n> \n> The helper is supposed to finish the fast-import stream with \"done\".\n> The documentation doesn't say much, but it also seems like the helper\n> could theoretically continue to respond to commands it's sent after\n> having done so, but that currently never happens AFAICT.\n\nHmm. We do not even pass --done to fast-import. If we are really\nexpecting everybody to say \"done\", then it seems like we ought to be\ndoing so. I think that \"done\" came much later than the concept of\nfast-import, so while most reasonable importers would send it, I suspect\nantique ones would not.\n\nSo I was all ready to say that we need to do it the other way (pass off\nownership) in order for fast-import to exit when the helper closes the\ndescriptor. But actually, I think I am being silly. The duplicated\ndescriptor is the _output_ from the helper, not the _input_ to\nfast-import. So if we are also holding that output descriptor,\nfast-import will not care. It is only the helper which would then not\nnotice fast-import dying (and continue writing to the descriptor\nwithout EPIPE, since we are still on the other end of it).\n\nI think that's probably OK, as we would see fast-import exit and then\ncontinue ourselves. We'd probably die() immediately assuming fast-import\nexits with an error. But if we don't, want happens? The helper would\neventually block if it fills the pipe buffer. We'd eventually end up in\ndisconnect_helper(). I think it would work out because we\nclose(data->helper->out) before trying to reap the child, so it would\nget SIGPIPE then and exit.\n\nSo I think that works. But I have to admit that handing off ownership\nseems simpler to reason about. :)\n\nTotally orthogonal, but I think we might also want to introduce a helper\ncapability so that import helpers can say \"I always send 'done' to\nfast-import\". And then we can pass \"--done\" to fast-import, which means\nit would detect a truncated stream.\n\n-Peff\n"},{"id":"375708","messageId":"20190516083519.5n6qmdw2aaqhjudd@glandium.org","threadId":"51099","inReplyTo":"20190516032802.GB4596@sigill.intra.peff.net","subject":"Re: Closing fds twice when using remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-16T08:35:19Z","receivedAt":"2019-05-16T08:35:26Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, May 15, 2019 at 11:28:02PM -0400, Jeff King wrote:\n> Totally orthogonal, but I think we might also want to introduce a helper\n> capability so that import helpers can say \"I always send 'done' to\n> fast-import\". And then we can pass \"--done\" to fast-import, which means\n> it would detect a truncated stream.\n\nDoubly orthogonal, but I wouldn't mind a helper capability that allows\nimport helpers to deal with creating git objects on their own rather\nthan having a fast-import spawned (git-cinnabar actually doesn't use the\nfast-import stream it's offered to use, but can't be a fetch helper\neither)\n\nMike\n"},{"id":"375746","messageId":"20190516214752.GE10787@sigill.intra.peff.net","threadId":"51099","inReplyTo":"20190516083519.5n6qmdw2aaqhjudd@glandium.org","subject":"Re: Closing fds twice when using remote helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-16T21:47:52Z","receivedAt":"2019-05-16T21:47:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 16, 2019 at 05:35:19PM +0900, Mike Hommey wrote:\n\n> On Wed, May 15, 2019 at 11:28:02PM -0400, Jeff King wrote:\n> > Totally orthogonal, but I think we might also want to introduce a helper\n> > capability so that import helpers can say \"I always send 'done' to\n> > fast-import\". And then we can pass \"--done\" to fast-import, which means\n> > it would detect a truncated stream.\n> \n> Doubly orthogonal, but I wouldn't mind a helper capability that allows\n> import helpers to deal with creating git objects on their own rather\n> than having a fast-import spawned (git-cinnabar actually doesn't use the\n> fast-import stream it's offered to use, but can't be a fetch helper\n> either)\n\nYeah, while writing that I definitely thought \"Gee, wouldn't it be\neasier if the importer was just responsible for running fast-import\nitself?\".\n\nWhat makes it impossible to run as a normal fetch-helper? Is it that we\nexpect the helper to then report back the refs for us to update?\n\nSo I take it your strategy is to just run your own fast-import, and then\nyou just pass along EOF with no input to the one that Git runs, to\nsignal that you're done importing? So maybe the right capability is to\nlet the helper say \"by the way, I don't need you to run fast-import for\nme\". But it's probably not that big a deal, since it's just wasting an\nextra process startup.\n\n-Peff\n"},{"id":"375749","messageId":"20190516220210.6abbye45p6yt5dol@glandium.org","threadId":"51099","inReplyTo":"20190516214752.GE10787@sigill.intra.peff.net","subject":"Re: Closing fds twice when using remote helpers","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-05-16T22:02:10Z","receivedAt":"2019-05-16T22:02:17Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, May 16, 2019 at 05:47:52PM -0400, Jeff King wrote:\n> On Thu, May 16, 2019 at 05:35:19PM +0900, Mike Hommey wrote:\n> \n> > On Wed, May 15, 2019 at 11:28:02PM -0400, Jeff King wrote:\n> > > Totally orthogonal, but I think we might also want to introduce a helper\n> > > capability so that import helpers can say \"I always send 'done' to\n> > > fast-import\". And then we can pass \"--done\" to fast-import, which means\n> > > it would detect a truncated stream.\n> > \n> > Doubly orthogonal, but I wouldn't mind a helper capability that allows\n> > import helpers to deal with creating git objects on their own rather\n> > than having a fast-import spawned (git-cinnabar actually doesn't use the\n> > fast-import stream it's offered to use, but can't be a fetch helper\n> > either)\n> \n> Yeah, while writing that I definitely thought \"Gee, wouldn't it be\n> easier if the importer was just responsible for running fast-import\n> itself?\".\n> \n> What makes it impossible to run as a normal fetch-helper? Is it that we\n> expect the helper to then report back the refs for us to update?\n> \n> So I take it your strategy is to just run your own fast-import, and then\n> you just pass along EOF with no input to the one that Git runs, to\n> signal that you're done importing? So maybe the right capability is to\n> let the helper say \"by the way, I don't need you to run fast-import for\n> me\". But it's probably not that big a deal, since it's just wasting an\n> extra process startup.\n\nWhen it received the \"import\" command, git-cinnabar does its stuff,\ncreates the namespaced refs it advertized in its response to\n\"capabilities\", and writes \"done\". That's all. So in fact, the current\nsetup, without --done passed to fast-import, just works.\n\nMike\n"}]}