{"thread":{"id":"62003","subject":"[PATCH] bundle-uri: plug leak in unbundle_from_file()","startedAt":"2024-08-26T08:32:14Z","lastAt":"2024-10-10T18:48:00Z","messageCount":9,"participants":["Toon Claes","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"501665","messageId":"20240826083052.1542228-1-toon@iotcl.com","threadId":"62003","inReplyTo":null,"subject":"[PATCH] bundle-uri: plug leak in unbundle_from_file()","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-08-26T08:30:52Z","receivedAt":"2024-08-26T08:32:14Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"When the function returns early, the variable bundle_ref is not released\nthrough strbuf_release().\n\nFix this leak. And while at it, remove assignments in the conditions of\nthe \"if\" statements as suggested in the CodingGuidelines.\n\nSigned-off-by: Toon Claes \n---\n bundle-uri.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 1e0ee156ba..eb49cba182 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -367,17 +367,21 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \tstruct strbuf bundle_ref = STRBUF_INIT;\n \tsize_t bundle_prefix_len;\n\n-\tif ((bundle_fd = read_bundle_header(file, &header)) < 0)\n-\t\treturn 1;\n+\tbundle_fd = read_bundle_header(file, &header);\n+\tif (bundle_fd < 0) {\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n\n \t/*\n \t * Skip the reachability walk here, since we will be adding\n \t * a reachable ref pointing to the new tips, which will reach\n \t * the prerequisite commits.\n \t */\n-\tif ((result = unbundle(r, &header, bundle_fd, NULL,\n-\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0))))\n-\t\treturn 1;\n+\tresult = unbundle(r, &header, bundle_fd, NULL,\n+\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n+\tif (result)\n+\t\tgoto cleanup;\n\n \t/*\n \t * Convert all refs/heads/ from the bundle into refs/bundles/\n@@ -406,6 +410,8 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \t\t\t\t0, UPDATE_REFS_MSG_ON_ERR);\n \t}\n\n+cleanup:\n+\tstrbuf_release(&bundle_ref);\n \tbundle_header_release(&header);\n \treturn result;\n }\n--\n2.46.0\n"},{"id":"501667","messageId":"ZsxQBEpfChQozhF7@tanuki","threadId":"62003","inReplyTo":"20240826083052.1542228-1-toon@iotcl.com","subject":"Re: [PATCH] bundle-uri: plug leak in unbundle_from_file()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-26T09:51:00Z","receivedAt":"2024-08-26T09:51:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 26, 2024 at 10:30:52AM +0200, Toon Claes wrote:\n> When the function returns early, the variable bundle_ref is not released\n> through strbuf_release().\n> \n> Fix this leak. And while at it, remove assignments in the conditions of\n> the \"if\" statements as suggested in the CodingGuidelines.\n> \n> Signed-off-by: Toon Claes \n> ---\n> bundle-uri.c | 16 +++++++++++-----\n> 1 file changed, 11 insertions(+), 5 deletions(-)\n> \n> diff --git a/bundle-uri.c b/bundle-uri.c\n> index 1e0ee156ba..eb49cba182 100644\n> --- a/bundle-uri.c\n> +++ b/bundle-uri.c\n> @@ -367,17 +367,21 @@ static int unbundle_from_file(struct repository *r, const char *file)\n> \tstruct strbuf bundle_ref = STRBUF_INIT;\n> \tsize_t bundle_prefix_len;\n> \n> -\tif ((bundle_fd = read_bundle_header(file, &header)) < 0)\n> -\t\treturn 1;\n> +\tbundle_fd = read_bundle_header(file, &header);\n> +\tif (bundle_fd < 0) {\n> +\t\tresult = 1;\n> +\t\tgoto cleanup;\n> +\t}\n> \n> \t/*\n> \t * Skip the reachability walk here, since we will be adding\n> \t * a reachable ref pointing to the new tips, which will reach\n> \t * the prerequisite commits.\n> \t */\n> -\tif ((result = unbundle(r, &header, bundle_fd, NULL,\n> -\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0))))\n> -\t\treturn 1;\n> +\tresult = unbundle(r, &header, bundle_fd, NULL,\n> +\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n> +\tif (result)\n> +\t\tgoto cleanup;\n\nThis changes the returned error code from `1` to whatever `unbundle()`\nreturns. Is this intentional? If so, the commit message should explain\nwhy this change is safe.\n\nOther than that this looks good to me, and the fix does not conflict\nwith any of my leak-plugging series.\n\nThanks!\n\nPatrick\n"},{"id":"501674","messageId":"xmqqle0jmfpf.fsf@gitster.g","threadId":"62003","inReplyTo":"ZsxQBEpfChQozhF7@tanuki","subject":"Re: [PATCH] bundle-uri: plug leak in unbundle_from_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-26T16:06:20Z","receivedAt":"2024-08-26T16:06:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Aug 26, 2024 at 10:30:52AM +0200, Toon Claes wrote:\n>> When the function returns early, the variable bundle_ref is not released\n>> through strbuf_release().\n>> \n>> Fix this leak. And while at it, remove assignments in the conditions of\n>> the \"if\" statements as suggested in the CodingGuidelines.\n> ...\n>> -\tif ((result = unbundle(r, &header, bundle_fd, NULL,\n>> -\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0))))\n>> -\t\treturn 1;\n>> +\tresult = unbundle(r, &header, bundle_fd, NULL,\n>> +\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n>> +\tif (result)\n>> +\t\tgoto cleanup;\n>\n> This changes the returned error code from `1` to whatever `unbundle()`\n> returns. Is this intentional? If so, the commit message should explain\n> why this change is safe.\n\nThanks for reviewing carefully.\n\nBoth of two callers of unbundle_from_file() are used as the\ncondition of an if() statement, so unbundle() that signals an error\nwith -1 wouldn't be a problem, I would think.\n\nIt may not be a bad idea as a #leftoverbits item, after the dust\nsettles, to clean up the calling convention in this file (may not be\nlimited to the code path that reaches this function) to follow the\nusual \"signal success with 0, failures are signalled with a negative\nvalue\". Then we can just return the value we got from a failing\nread_bundle_header(), just the same way we return the value we got\nfrom a failing unbundle().\n\n> Other than that this looks good to me, and the fix does not conflict\n> with any of my leak-plugging series.\n\nYup. Thanks, both.\n"},{"id":"503860","messageId":"87ikubsl9a.fsf@iotcl.com","threadId":"62003","inReplyTo":"xmqqle0jmfpf.fsf@gitster.g","subject":"Re: [PATCH] bundle-uri: plug leak in unbundle_from_file()","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-10-01T18:58:41Z","receivedAt":"2024-10-01T18:58:58Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Junio C Hamano writes:\n\n> Thanks for reviewing carefully.\n>\n> Both of two callers of unbundle_from_file() are used as the\n> condition of an if() statement, so unbundle() that signals an error\n> with -1 wouldn't be a problem, I would think.\n\nHi Junio,\n\nI've noticed this patch wasn't picked up yet. Is there anything you want\nme to change and have me sent another version, or is it good to go in?\n\n--\nToon\n"},{"id":"503867","messageId":"xmqqfrpfvcyu.fsf@gitster.g","threadId":"62003","inReplyTo":"87ikubsl9a.fsf@iotcl.com","subject":"Re: [PATCH] bundle-uri: plug leak in unbundle_from_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-01T19:29:29Z","receivedAt":"2024-10-01T19:29:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes writes:\n\n> Junio C Hamano writes:\n>\n>> Thanks for reviewing carefully.\n>>\n>> Both of two callers of unbundle_from_file() are used as the\n>> condition of an if() statement, so unbundle() that signals an error\n>> with -1 wouldn't be a problem, I would think.\n>\n> Hi Junio,\n>\n> I've noticed this patch wasn't picked up yet. Is there anything you want\n> me to change and have me sent another version, or is it good to go in?\n\nI am waiting for a reroll with an updated log message, i.e, what\nPatrick pointed out in his review. I only said \"yeah, this looks\nsafe\", and never meant \"it is so obvious there is no need for extra\nexplanation in the log message\".\n\nThanks.\n"},{"id":"504685","messageId":"20241010091249.1895960-1-toon@iotcl.com","threadId":"62003","inReplyTo":"20240826083052.1542228-1-toon@iotcl.com","subject":"[PATCH v2] bundle-uri: plug leak in unbundle_from_file()","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-10-10T09:12:49Z","receivedAt":"2024-10-10T09:13:04Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"The function `unbundle_from_file()` has two memory leaks:\n\n - We do not release the `struct bundle_header header` when hitting\n errors because we return early without any cleanup.\n\n - We do not release the `struct strbuf bundle_ref` at all.\n\nPlug these leaks by creating a common exit path where both of these\nvariables are released.\n\nWhile at it, refactor the code such that the variable assignments do not\nhappen inside the conditional statement itself according to our coding\nstyle.\n\nSigned-off-by: Toon Claes \n---\n bundle-uri.c | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle-uri.c b/bundle-uri.c\nindex 4b1a2e2937..0df66e2872 100644\n--- a/bundle-uri.c\n+++ b/bundle-uri.c\n@@ -368,17 +368,23 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \tstruct strbuf bundle_ref = STRBUF_INIT;\n \tsize_t bundle_prefix_len;\n \n-\tif ((bundle_fd = read_bundle_header(file, &header)) < 0)\n-\t\treturn 1;\n+\tbundle_fd = read_bundle_header(file, &header);\n+\tif (bundle_fd < 0) {\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n \n \t/*\n \t * Skip the reachability walk here, since we will be adding\n \t * a reachable ref pointing to the new tips, which will reach\n \t * the prerequisite commits.\n \t */\n-\tif ((result = unbundle(r, &header, bundle_fd, NULL,\n-\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0))))\n-\t\treturn 1;\n+\tresult = unbundle(r, &header, bundle_fd, NULL,\n+\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n+\tif (result) {\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n \n \t/*\n \t * Convert all refs/heads/ from the bundle into refs/bundles/\n@@ -407,6 +413,8 @@ static int unbundle_from_file(struct repository *r, const char *file)\n \t\t\t\t0, UPDATE_REFS_MSG_ON_ERR);\n \t}\n \n+cleanup:\n+\tstrbuf_release(&bundle_ref);\n \tbundle_header_release(&header);\n \treturn result;\n }\n\nRange-diff against v1:\n1: f30f393e05 ! 1: 19714d860c bundle-uri: plug leak in unbundle_from_file()\n @@ Metadata\n ## Commit message ##\n bundle-uri: plug leak in unbundle_from_file()\n \n - When the function returns early, the variable bundle_ref is not released\n - through strbuf_release().\n + The function `unbundle_from_file()` has two memory leaks:\n \n - Fix this leak. And while at it, remove assignments in the conditions of\n - the \"if\" statements as suggested in the CodingGuidelines.\n + - We do not release the `struct bundle_header header` when hitting\n + errors because we return early without any cleanup.\n +\n + - We do not release the `struct strbuf bundle_ref` at all.\n +\n + Plug these leaks by creating a common exit path where both of these\n + variables are released.\n +\n + While at it, refactor the code such that the variable assignments do not\n + happen inside the conditional statement itself according to our coding\n + style.\n \n Signed-off-by: Toon Claes \n \n @@ bundle-uri.c: static int unbundle_from_file(struct repository *r, const char *fi\n -\t\treturn 1;\n +\tresult = unbundle(r, &header, bundle_fd, NULL,\n +\t\t\t VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));\n -+\tif (result)\n ++\tif (result) {\n ++\t\tresult = 1;\n +\t\tgoto cleanup;\n ++\t}\n \n \t/*\n \t * Convert all refs/heads/ from the bundle into refs/bundles/\n-- \n2.46.0.46.g406f326d27\n\n"},{"id":"504686","messageId":"875xq0s4in.fsf@iotcl.com","threadId":"62003","inReplyTo":"xmqqle0jmfpf.fsf@gitster.g","subject":"Re: [PATCH] bundle-uri: plug leak in unbundle_from_file()","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-10-10T09:15:12Z","receivedAt":"2024-10-10T09:15:27Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Junio C Hamano writes:\n\n> Both of two callers of unbundle_from_file() are used as the\n> condition of an if() statement, so unbundle() that signals an error\n> with -1 wouldn't be a problem, I would think.\n\n> It may not be a bad idea as a #leftoverbits item, after the dust\n> settles, to clean up the calling convention in this file (may not be\n> limited to the code path that reaches this function) to follow the\n> usual \"signal success with 0, failures are signalled with a negative\n> value\". Then we can just return the value we got from a failing\n> read_bundle_header(), just the same way we return the value we got\n> from a failing unbundle().\n\nI just submitted v2 which preserves the original return values as\nbefore. I'll leave changing any return value to a #leftoverbits series\nto addresses them together.\n\n--\nToon\n"},{"id":"504689","messageId":"ZwfOsoC35x-wyn7S@pks.im","threadId":"62003","inReplyTo":"20241010091249.1895960-1-toon@iotcl.com","subject":"Re: [PATCH v2] bundle-uri: plug leak in unbundle_from_file()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-10T12:55:14Z","receivedAt":"2024-10-10T12:55:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 10, 2024 at 11:12:49AM +0200, Toon Claes wrote:\n> The function `unbundle_from_file()` has two memory leaks:\n> \n> - We do not release the `struct bundle_header header` when hitting\n> errors because we return early without any cleanup.\n> \n> - We do not release the `struct strbuf bundle_ref` at all.\n> \n> Plug these leaks by creating a common exit path where both of these\n> variables are released.\n> \n> While at it, refactor the code such that the variable assignments do not\n> happen inside the conditional statement itself according to our coding\n> style.\n\nThanks, this version looks good to me. We now avoid any discussion\naround the changed error code completely, and the commit message seems\nreasonable to me.\n\nThanks!\n\nPatrick\n"},{"id":"504731","messageId":"xmqq8quvlrqa.fsf@gitster.g","threadId":"62003","inReplyTo":"ZwfOsoC35x-wyn7S@pks.im","subject":"Re: [PATCH v2] bundle-uri: plug leak in unbundle_from_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-10T18:47:57Z","receivedAt":"2024-10-10T18:48:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Oct 10, 2024 at 11:12:49AM +0200, Toon Claes wrote:\n>> The function `unbundle_from_file()` has two memory leaks:\n>> \n>> - We do not release the `struct bundle_header header` when hitting\n>> errors because we return early without any cleanup.\n>> \n>> - We do not release the `struct strbuf bundle_ref` at all.\n>> \n>> Plug these leaks by creating a common exit path where both of these\n>> variables are released.\n>> \n>> While at it, refactor the code such that the variable assignments do not\n>> happen inside the conditional statement itself according to our coding\n>> style.\n>\n> Thanks, this version looks good to me. We now avoid any discussion\n> around the changed error code completely, and the commit message seems\n> reasonable to me.\n>\n> Thanks!\n>\n> Patrick\n\nThanks, both of you.\n\nWill queue.\n"}]}