{"thread":{"id":"61652","subject":"Ownership handling of the \"remote name\" buffer given to `remote_get` is inconsistent","startedAt":"2024-06-19T20:14:45Z","lastAt":"2024-06-19T21:24:57Z","messageCount":3,"participants":["Mike Hommey","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"497371","messageId":"20240619195311.yhmbv5ruixa433rn@glandium.org","threadId":"61652","inReplyTo":null,"subject":"Ownership handling of the \"remote name\" buffer given to `remote_get` is inconsistent","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2024-06-19T19:53:11Z","receivedAt":"2024-06-19T20:14:45Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nI actually found this a few years ago, but didn't worry too much about\nit, forgot about it, then rediscovered it yesterday. I tried to untangle\nthe situation, but it looks like it would take me more time than I can\nallocate, so I'm sending this report.\n\nWhen you call `remote_get(name)`, you may end up giving ownership of\n`name` to the remote, such that if you `free(name)`, using the remote\nafter that may lead to use-after-free. Of course, this is a\nuse-libgit-as-a-library situation; I don't think it happens in actual\ngit code.\n\nHere's how it goes: if your `name` is actually a URL, you end up in this\ncase in remotes_remote_get_1:\n\n        if (name_given && !valid_remote(ret))\n                add_url_alias(remote_state, ret, name);\n\n`add_url_alias` will eventually set `ret->url` to contain `name` as-is,\ntaking ownership. Later `release_clear` will free it, which will lead to\ndouble-free if the caller frees it.\n\nIt's further complicated by `add_url_alias` also sometimes *not* taking\nownership, in the face of URL rewrites (in `alias_url`). Meaning, the\nfix is not as straightforward as changing the above to\n\n                add_url_alias(remote_state, ret, xstrdup(name));\n\nThis is as far as I went before my time was up on this issue.\n\nMike\n"},{"id":"497372","messageId":"20240619204450.GA1527089@coredump.intra.peff.net","threadId":"61652","inReplyTo":"20240619195311.yhmbv5ruixa433rn@glandium.org","subject":"Re: Ownership handling of the \"remote name\" buffer given to `remote_get` is inconsistent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-19T20:44:50Z","receivedAt":"2024-06-19T20:44:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2024 at 04:53:11AM +0900, Mike Hommey wrote:\n\n> When you call `remote_get(name)`, you may end up giving ownership of\n> `name` to the remote, such that if you `free(name)`, using the remote\n> after that may lead to use-after-free. Of course, this is a\n> use-libgit-as-a-library situation; I don't think it happens in actual\n> git code.\n\nTry this recent series:\n\n  https://lore.kernel.org/git/20240614102439.GA222287@coredump.intra.peff.net/\n\nespecially patches 2 and 3. Junio has it queued as jk/remote-wo-url.\nAlso, you might want to check the later patches to make sure the\ncleanups don't cause any problems for your use case.\n\n-Peff\n"},{"id":"497373","messageId":"20240619212448.c66nvf6ca745yvux@glandium.org","threadId":"61652","inReplyTo":"20240619204450.GA1527089@coredump.intra.peff.net","subject":"Re: Ownership handling of the \"remote name\" buffer given to `remote_get` is inconsistent","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2024-06-19T21:24:48Z","receivedAt":"2024-06-19T21:24:57Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Wed, Jun 19, 2024 at 04:44:50PM -0400, Jeff King wrote:\n> On Thu, Jun 20, 2024 at 04:53:11AM +0900, Mike Hommey wrote:\n> \n> > When you call `remote_get(name)`, you may end up giving ownership of\n> > `name` to the remote, such that if you `free(name)`, using the remote\n> > after that may lead to use-after-free. Of course, this is a\n> > use-libgit-as-a-library situation; I don't think it happens in actual\n> > git code.\n> \n> Try this recent series:\n> \n>   https://lore.kernel.org/git/20240614102439.GA222287@coredump.intra.peff.net/\n> \n> especially patches 2 and 3. Junio has it queued as jk/remote-wo-url.\n> Also, you might want to check the later patches to make sure the\n> cleanups don't cause any problems for your use case.\n\nWhat timing! It looks like both these two patches address my issue\nwithout breaking anything for me.\n\nThanks\n\nMike\n"}]}