{"thread":{"id":"62061","subject":"[PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","startedAt":"2024-09-05T08:27:56Z","lastAt":"2024-09-06T14:55:12Z","messageCount":12,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"502199","messageId":"20240905082749.GA88220@coredump.intra.peff.net","threadId":"62061","inReplyTo":null,"subject":"[PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-05T08:27:49Z","receivedAt":"2024-09-05T08:27:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When updating sparse patterns, we open a lock_file to write out the new\ndata. The lock_file struct holds the file descriptor, but we call\nfdopen() to get a stdio handle to do the actual write.\n\nAfter we finish writing, we fflush() so that all of the data is on disk,\nand then call commit_lock_file() which closes the descriptor. But we\nnever fclose() the stdio handle, leaking it.\n\nThe obvious solution seems like it would be to just call fclose(). But\nwhen? If we do it before commit_lock_file(), then the lock_file code is\nleft thinking it owns the now-closed file descriptor, and will do an\nextra close() on the descriptor. But if we do it before, we have the\nopposite problem: the lock_file code will close the descriptor, and\nfclose() will do the extra close().\n\nWe can handle this correctly by using fdopen_lock_file(). That leaves\nownership of the stdio handle with the lock_file, which knows not to\ndouble-close it.\n\nWe do have to adjust the code a bit:\n\n  - we have to handle errors ourselves; we can just die(), since that's\n    what xfdopen() would have done (and we can even provide a more\n    specific error message).\n\n  - we no longer need to call fflush(); committing the lock-file\n    auto-closes it, which will now do the flush for us. As a bonus, this\n    will actually check that the flush was successful before renaming\n    the file into place. Let's likewise report when committing the lock\n    fails (rather than quietly returning success from the command).\n\n  - we can get rid of the local \"fd\" variable, since we never look at it\n    ourselves now\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nCuriously, this is not reported as a leak by LSan, and it is included in\nthe \"reachable\" set by valgrind. I imagine this is because libc has to\nmaintain a list of open writable handles, since fflush(NULL) is supposed\nto flush all of them. I peeked at other fdopen(..., \"w\") calls to see if\nthere were other similar spots, but didn't notice any.\n\nI found this because I was building git on an Android system, and they\nhave an \"fdsan\" that complains when the underlying descriptor of a\nhandle is closed. It flagged some other spots, too, but the rest that I\nlooked at involved the tempfile atexit() handler closing the descriptors\nmanually (we don't care about flushing there, as our goal is to just\nprevent the \"can't delete an open file\" problem on Windows).\n\n builtin/sparse-checkout.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 2604ab04df..f1bd31b2f7 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -327,7 +327,6 @@ static int write_patterns_and_update(struct pattern_list *pl)\n {\n \tchar *sparse_filename;\n \tFILE *fp;\n-\tint fd;\n \tstruct lock_file lk = LOCK_INIT;\n \tint result;\n \n@@ -336,8 +335,7 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \tif (safe_create_leading_directories(sparse_filename))\n \t\tdie(_(\"failed to create directory for sparse-checkout file\"));\n \n-\tfd = hold_lock_file_for_update(&lk, sparse_filename,\n-\t\t\t\t      LOCK_DIE_ON_ERROR);\n+\thold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);\n \tfree(sparse_filename);\n \n \tresult = update_working_directory(pl);\n@@ -348,15 +346,17 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \t\treturn result;\n \t}\n \n-\tfp = xfdopen(fd, \"w\");\n+\tfp = fdopen_lock_file(&lk, \"w\");\n+\tif (!fp)\n+\t\tdie_errno(_(\"unable to fdopen %s\"), get_lock_file_path(&lk));\n \n \tif (core_sparse_checkout_cone)\n \t\twrite_cone_to_file(fp, pl);\n \telse\n \t\twrite_patterns_to_file(fp, pl);\n \n-\tfflush(fp);\n-\tcommit_lock_file(&lk);\n+\tif (commit_lock_file(&lk))\n+\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n \n \tclear_pattern_list(pl);\n \n-- \n2.46.0.802.g13da1a47c4\n"},{"id":"502233","messageId":"ZtmN6sZCvDy1BGfn@pks.im","threadId":"62061","inReplyTo":"20240905082749.GA88220@coredump.intra.peff.net","subject":"Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-05T10:54:42Z","receivedAt":"2024-09-05T10:54:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 05, 2024 at 04:27:49AM -0400, Jeff King wrote:\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index 2604ab04df..f1bd31b2f7 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -336,8 +335,7 @@ static int write_patterns_and_update(struct pattern_list *pl)\n>  \tif (safe_create_leading_directories(sparse_filename))\n>  \t\tdie(_(\"failed to create directory for sparse-checkout file\"));\n>  \n> -\tfd = hold_lock_file_for_update(&lk, sparse_filename,\n> -\t\t\t\t      LOCK_DIE_ON_ERROR);\n> +\thold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);\n>  \tfree(sparse_filename);\n>  \n>  \tresult = update_working_directory(pl);\n\nOkay, we die on an error so there is no need to check `fd`, either.\nMakes sense.\n\n> @@ -348,15 +346,17 @@ static int write_patterns_and_update(struct pattern_list *pl)\n>  \t\treturn result;\n>  \t}\n>  \n> -\tfp = xfdopen(fd, \"w\");\n> +\tfp = fdopen_lock_file(&lk, \"w\");\n> +\tif (!fp)\n> +\t\tdie_errno(_(\"unable to fdopen %s\"), get_lock_file_path(&lk));\n>  \n>  \tif (core_sparse_checkout_cone)\n>  \t\twrite_cone_to_file(fp, pl);\n>  \telse\n>  \t\twrite_patterns_to_file(fp, pl);\n>  \n> -\tfflush(fp);\n> -\tcommit_lock_file(&lk);\n> +\tif (commit_lock_file(&lk))\n> +\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n>  \n>  \tclear_pattern_list(pl);\n\nI think the error handling is broken. `commit_lock_file()` calls\n`rename_tempfile()`, which deletes the temporary file even in the error\ncase. The consequence is that `lk->tempfile` will be set to the `NULL`\npointer. When we call `get_locked_file_path()` we then dereference it\nunconditionally and would thus segfault.\n\nOther than that this patch looks sensible. Also kind of curious that we\nnever checked that `commit_lock_file()` was successful, so we wouldn't\neven notice when renaming the file into place failed.\n\nPatrick\n"},{"id":"502249","messageId":"xmqqle065dxm.fsf@gitster.g","threadId":"62061","inReplyTo":"20240905082749.GA88220@coredump.intra.peff.net","subject":"Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-05T15:16:05Z","receivedAt":"2024-09-05T15:16:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We do have to adjust the code a bit:\n>\n>   - we have to handle errors ourselves; we can just die(), since that's\n>     what xfdopen() would have done (and we can even provide a more\n>     specific error message).\n>\n>   - we no longer need to call fflush(); committing the lock-file\n>     auto-closes it, which will now do the flush for us. As a bonus, this\n>     will actually check that the flush was successful before renaming\n>     the file into place. Let's likewise report when committing the lock\n>     fails (rather than quietly returning success from the command).\n>\n>   - we can get rid of the local \"fd\" variable, since we never look at it\n>     ourselves now\n\nOK.  The neessary change is surprisingly small.\n\n> I found this because I was building git on an Android system,...\n\nSounds like fun.\n\nWill queue.  Thanks.\n"},{"id":"502286","messageId":"xmqqr09xztqa.fsf@gitster.g","threadId":"62061","inReplyTo":"ZtmN6sZCvDy1BGfn@pks.im","subject":"Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-05T21:16:45Z","receivedAt":"2024-09-05T21:16:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> +\tif (commit_lock_file(&lk))\n>> +\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n>>  \n>>  \tclear_pattern_list(pl);\n>\n> I think the error handling is broken. `commit_lock_file()` calls\n> `rename_tempfile()`, which deletes the temporary file even in the error\n> case. The consequence is that `lk->tempfile` will be set to the `NULL`\n> pointer. When we call `get_locked_file_path()` we then dereference it\n> unconditionally and would thus segfault.\n\nHmph.  Would this be sufficient as a band-aid, then?\n\n builtin/sparse-checkout.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/builtin/sparse-checkout.c w/builtin/sparse-checkout.c\nindex f1bd31b2f7..60363fd056 100644\n--- c/builtin/sparse-checkout.c\n+++ w/builtin/sparse-checkout.c\n@@ -356,7 +356,7 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \t\twrite_patterns_to_file(fp, pl);\n \n \tif (commit_lock_file(&lk))\n-\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n+\t\tdie_errno(_(\"unable to write %s\"), sparse_filename);\n \n \tclear_pattern_list(pl);\n \n"},{"id":"502287","messageId":"xmqqmsklztkd.fsf@gitster.g","threadId":"62061","inReplyTo":"xmqqr09xztqa.fsf@gitster.g","subject":"Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-05T21:20:18Z","receivedAt":"2024-09-05T21:20:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>>> +\tif (commit_lock_file(&lk))\n>>> +\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n>>>  \n>>>  \tclear_pattern_list(pl);\n>>\n>> I think the error handling is broken. `commit_lock_file()` calls\n>> `rename_tempfile()`, which deletes the temporary file even in the error\n>> case. The consequence is that `lk->tempfile` will be set to the `NULL`\n>> pointer. When we call `get_locked_file_path()` we then dereference it\n>> unconditionally and would thus segfault.\n>\n> Hmph.  Would this be sufficient as a band-aid, then?\n\nOf course not.  That would refer to a piece of memory that we\nalready free'ed in this function.\n\nPerhaps like this?\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex f1bd31b2f7..27d181a612 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -328,7 +328,7 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \tchar *sparse_filename;\n \tFILE *fp;\n \tstruct lock_file lk = LOCK_INIT;\n-\tint result;\n+\tint result = 0;\n \n \tsparse_filename = get_sparse_checkout_filename();\n \n@@ -336,19 +336,18 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \t\tdie(_(\"failed to create directory for sparse-checkout file\"));\n \n \thold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);\n-\tfree(sparse_filename);\n \n \tresult = update_working_directory(pl);\n \tif (result) {\n \t\trollback_lock_file(&lk);\n \t\tclear_pattern_list(pl);\n \t\tupdate_working_directory(NULL);\n-\t\treturn result;\n+\t\tgoto out;\n \t}\n \n \tfp = fdopen_lock_file(&lk, \"w\");\n \tif (!fp)\n-\t\tdie_errno(_(\"unable to fdopen %s\"), get_lock_file_path(&lk));\n+\t\tdie_errno(_(\"unable to fdopen %s\"), sparse_filename);\n \n \tif (core_sparse_checkout_cone)\n \t\twrite_cone_to_file(fp, pl);\n@@ -356,11 +355,13 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \t\twrite_patterns_to_file(fp, pl);\n \n \tif (commit_lock_file(&lk))\n-\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n+\t\tdie_errno(_(\"unable to write %s\"), sparse_filename);\n \n \tclear_pattern_list(pl);\n \n-\treturn 0;\n+out:\n+\tfree(sparse_filename);\n+\treturn result;\n }\n \n enum sparse_checkout_mode {\n"},{"id":"502296","messageId":"20240906011935.GA2616101@coredump.intra.peff.net","threadId":"62061","inReplyTo":"xmqqmsklztkd.fsf@gitster.g","subject":"Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-06T01:19:35Z","receivedAt":"2024-09-06T01:19:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 05, 2024 at 02:20:18PM -0700, Junio C Hamano wrote:\n\n> >> I think the error handling is broken. `commit_lock_file()` calls\n> >> `rename_tempfile()`, which deletes the temporary file even in the error\n> >> case. The consequence is that `lk->tempfile` will be set to the `NULL`\n> >> pointer. When we call `get_locked_file_path()` we then dereference it\n> >> unconditionally and would thus segfault.\n> >\n> > Hmph.  Would this be sufficient as a band-aid, then?\n> \n> Of course not.  That would refer to a piece of memory that we\n> already free'ed in this function.\n> \n> Perhaps like this?\n\nThat works, though...\n\n>  \tfp = fdopen_lock_file(&lk, \"w\");\n>  \tif (!fp)\n> -\t\tdie_errno(_(\"unable to fdopen %s\"), get_lock_file_path(&lk));\n> +\t\tdie_errno(_(\"unable to fdopen %s\"), sparse_filename);\n>  \n>  \tif (core_sparse_checkout_cone)\n>  \t\twrite_cone_to_file(fp, pl);\n> @@ -356,11 +355,13 @@ static int write_patterns_and_update(struct pattern_list *pl)\n>  \t\twrite_patterns_to_file(fp, pl);\n>  \n>  \tif (commit_lock_file(&lk))\n> -\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n> +\t\tdie_errno(_(\"unable to write %s\"), sparse_filename);\n\nNote the difference between \"get_lock\" and \"get_locked\" in these two.\nThe first will mention the tempfile name, and the second the destination\nfilename (and sparse_filename is the latter).\n\nI don't know that it really matters much in practice, though. If\nfdopen() fails it probably has nothing to do with the file itself, and\nis either lack of memory or a programming bug (e.g., bogus descriptor).\n\nI'll pick up this change, but I'll split the whole error-handling fix\ninto its own patch, since it's getting more complicated.\n\nWill send v2 later tonight. Thanks, Patrick, for noticing the problem in\nthe first place.\n\n-Peff\n"},{"id":"502297","messageId":"20240906034557.GA3693911@coredump.intra.peff.net","threadId":"62061","inReplyTo":"20240905082749.GA88220@coredump.intra.peff.net","subject":"[PATCH v2 0/3] sparse-checkout file handle leak fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-06T03:45:57Z","receivedAt":"2024-09-06T03:45:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a re-roll that fixes the use-after-free of the lock filename that\nPatrick noticed. I pulled the error-checking fix into its own patch\n(patch 2 here), and did Junio's suggested \"goto out\" as preparation in\npatch 1. Patch 3 is the leak fix.\n\nRange diff is below, but it's much harder to read than just looking at\nthe updated patch 3.\n\nThanks both for review on round 1.\n\n  [1/3]: sparse-checkout: consolidate cleanup when writing patterns\n  [2/3]: sparse-checkout: check commit_lock_file when writing patterns\n  [3/3]: sparse-checkout: use fdopen_lock_file() instead of xfdopen()\n\n builtin/sparse-checkout.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\n-:  ---------- > 1:  03a80d3748 sparse-checkout: consolidate cleanup when writing patterns\n-:  ---------- > 2:  20628aa350 sparse-checkout: check commit_lock_file when writing patterns\n1:  b4700ba1a9 ! 3:  fe41f9f02b sparse-checkout: use fdopen_lock_file() instead of xfdopen()\n    @@ Commit message\n           - we no longer need to call fflush(); committing the lock-file\n             auto-closes it, which will now do the flush for us. As a bonus, this\n             will actually check that the flush was successful before renaming\n    -        the file into place. Let's likewise report when committing the lock\n    -        fails (rather than quietly returning success from the command).\n    +        the file into place.\n     \n           - we can get rid of the local \"fd\" variable, since we never look at it\n             ourselves now\n    @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_l\n     -\tfd = hold_lock_file_for_update(&lk, sparse_filename,\n     -\t\t\t\t      LOCK_DIE_ON_ERROR);\n     +\thold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);\n    - \tfree(sparse_filename);\n      \n      \tresult = update_working_directory(pl);\n    + \tif (result) {\n     @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_list *pl)\n    - \t\treturn result;\n    + \t\tgoto out;\n      \t}\n      \n     -\tfp = xfdopen(fd, \"w\");\n    @@ builtin/sparse-checkout.c: static int write_patterns_and_update(struct pattern_l\n      \t\twrite_patterns_to_file(fp, pl);\n      \n     -\tfflush(fp);\n    --\tcommit_lock_file(&lk);\n    -+\tif (commit_lock_file(&lk))\n    -+\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n    - \n    - \tclear_pattern_list(pl);\n    + \tif (commit_lock_file(&lk))\n    + \t\tdie_errno(_(\"unable to write %s\"), sparse_filename);\n      \n\n"},{"id":"502298","messageId":"20240906034708.GA4168362@coredump.intra.peff.net","threadId":"62061","inReplyTo":"20240906034557.GA3693911@coredump.intra.peff.net","subject":"[PATCH v2 1/3] sparse-checkout: consolidate cleanup when writing patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-06T03:47:08Z","receivedAt":"2024-09-06T03:47:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In write_patterns_and_update(), we always need to free the pattern list\nbefore exiting the function.  Rather than handling it manually when we\nreturn early, we can jump to an \"out\" label where cleanup happens. This\nlet us drop one line, but also establishes a pattern we can use for\nother cleanup.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis would also be helpful in the long run if all of the other die()\ncalls ended up as error returns, but I guess this is in builtin/, so it\nmay not be that likely a candidate for libification anyway.\n\n builtin/sparse-checkout.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex 2604ab04df..dfefe609a1 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -343,9 +343,8 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \tresult = update_working_directory(pl);\n \tif (result) {\n \t\trollback_lock_file(&lk);\n-\t\tclear_pattern_list(pl);\n \t\tupdate_working_directory(NULL);\n-\t\treturn result;\n+\t\tgoto out;\n \t}\n \n \tfp = xfdopen(fd, \"w\");\n@@ -358,9 +357,9 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \tfflush(fp);\n \tcommit_lock_file(&lk);\n \n+out:\n \tclear_pattern_list(pl);\n-\n-\treturn 0;\n+\treturn result;\n }\n \n enum sparse_checkout_mode {\n-- \n2.46.0.802.g13da1a47c4\n\n"},{"id":"502299","messageId":"20240906034738.GB4168362@coredump.intra.peff.net","threadId":"62061","inReplyTo":"20240906034557.GA3693911@coredump.intra.peff.net","subject":"[PATCH v2 2/3] sparse-checkout: check commit_lock_file when writing patterns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-06T03:47:38Z","receivedAt":"2024-09-06T03:47:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When writing a new \"sparse-checkout\" file, we do the usual strategy of\nwriting to a lockfile and committing it into place. But we don't check\nthe outcome of commit_lock_file(). Failing there would prevent us from\nwriting a bogus file (good), but we would ignore the error and return a\nsuccessful exit code (bad).\n\nFix this by calling die(). Note that we need to keep the sparse_filename\nvariable valid for longer, since the filename stored in the lock_file\nstruct will be dropped when we run commit_lock_file().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/sparse-checkout.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex dfefe609a1..b5e220cc44 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -338,7 +338,6 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \n \tfd = hold_lock_file_for_update(&lk, sparse_filename,\n \t\t\t\t      LOCK_DIE_ON_ERROR);\n-\tfree(sparse_filename);\n \n \tresult = update_working_directory(pl);\n \tif (result) {\n@@ -355,10 +354,12 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \t\twrite_patterns_to_file(fp, pl);\n \n \tfflush(fp);\n-\tcommit_lock_file(&lk);\n+\tif (commit_lock_file(&lk))\n+\t\tdie_errno(_(\"unable to write %s\"), sparse_filename);\n \n out:\n \tclear_pattern_list(pl);\n+\tfree(sparse_filename);\n \treturn result;\n }\n \n-- \n2.46.0.802.g13da1a47c4\n\n"},{"id":"502300","messageId":"20240906034841.GC4168362@coredump.intra.peff.net","threadId":"62061","inReplyTo":"20240906034557.GA3693911@coredump.intra.peff.net","subject":"[PATCH v2 3/3] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-06T03:48:41Z","receivedAt":"2024-09-06T03:48:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When updating sparse patterns, we open a lock_file to write out the new\ndata. The lock_file struct holds the file descriptor, but we call\nfdopen() to get a stdio handle to do the actual write.\n\nAfter we finish writing, we fflush() so that all of the data is on disk,\nand then call commit_lock_file() which closes the descriptor. But we\nnever fclose() the stdio handle, leaking it.\n\nThe obvious solution seems like it would be to just call fclose(). But\nwhen? If we do it before commit_lock_file(), then the lock_file code is\nleft thinking it owns the now-closed file descriptor, and will do an\nextra close() on the descriptor. But if we do it before, we have the\nopposite problem: the lock_file code will close the descriptor, and\nfclose() will do the extra close().\n\nWe can handle this correctly by using fdopen_lock_file(). That leaves\nownership of the stdio handle with the lock_file, which knows not to\ndouble-close it.\n\nWe do have to adjust the code a bit:\n\n  - we have to handle errors ourselves; we can just die(), since that's\n    what xfdopen() would have done (and we can even provide a more\n    specific error message).\n\n  - we no longer need to call fflush(); committing the lock-file\n    auto-closes it, which will now do the flush for us. As a bonus, this\n    will actually check that the flush was successful before renaming\n    the file into place.\n\n  - we can get rid of the local \"fd\" variable, since we never look at it\n    ourselves now\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI left this error message using get_lock_file_path(&lk), which is still\nvalid, and is arguably correct than sparse_filename.\n\n builtin/sparse-checkout.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex b5e220cc44..5ccf696862 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -327,7 +327,6 @@ static int write_patterns_and_update(struct pattern_list *pl)\n {\n \tchar *sparse_filename;\n \tFILE *fp;\n-\tint fd;\n \tstruct lock_file lk = LOCK_INIT;\n \tint result;\n \n@@ -336,8 +335,7 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \tif (safe_create_leading_directories(sparse_filename))\n \t\tdie(_(\"failed to create directory for sparse-checkout file\"));\n \n-\tfd = hold_lock_file_for_update(&lk, sparse_filename,\n-\t\t\t\t      LOCK_DIE_ON_ERROR);\n+\thold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR);\n \n \tresult = update_working_directory(pl);\n \tif (result) {\n@@ -346,14 +344,15 @@ static int write_patterns_and_update(struct pattern_list *pl)\n \t\tgoto out;\n \t}\n \n-\tfp = xfdopen(fd, \"w\");\n+\tfp = fdopen_lock_file(&lk, \"w\");\n+\tif (!fp)\n+\t\tdie_errno(_(\"unable to fdopen %s\"), get_lock_file_path(&lk));\n \n \tif (core_sparse_checkout_cone)\n \t\twrite_cone_to_file(fp, pl);\n \telse\n \t\twrite_patterns_to_file(fp, pl);\n \n-\tfflush(fp);\n \tif (commit_lock_file(&lk))\n \t\tdie_errno(_(\"unable to write %s\"), sparse_filename);\n \n-- \n2.46.0.802.g13da1a47c4\n"},{"id":"502314","messageId":"ZtrKf4CW_vxJt9eA@pks.im","threadId":"62061","inReplyTo":"20240906034557.GA3693911@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/3] sparse-checkout file handle leak fix","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-06T09:25:24Z","receivedAt":"2024-09-06T09:25:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 05, 2024 at 11:45:57PM -0400, Jeff King wrote:\n> Here's a re-roll that fixes the use-after-free of the lock filename that\n> Patrick noticed. I pulled the error-checking fix into its own patch\n> (patch 2 here), and did Junio's suggested \"goto out\" as preparation in\n> patch 1. Patch 3 is the leak fix.\n> \n> Range diff is below, but it's much harder to read than just looking at\n> the updated patch 3.\n> \n> Thanks both for review on round 1.\n\nThanks for going the extra steps! This version looks good to me.\n\nPatrick\n"},{"id":"502329","messageId":"xmqq1q1wzvaq.fsf@gitster.g","threadId":"62061","inReplyTo":"20240906011935.GA2616101@coredump.intra.peff.net","subject":"Re: [PATCH] sparse-checkout: use fdopen_lock_file() instead of xfdopen()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-06T14:55:09Z","receivedAt":"2024-09-06T14:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>>  \tfp = fdopen_lock_file(&lk, \"w\");\n>>  \tif (!fp)\n>> -\t\tdie_errno(_(\"unable to fdopen %s\"), get_lock_file_path(&lk));\n>> +\t\tdie_errno(_(\"unable to fdopen %s\"), sparse_filename);\n>>  \n>>  \tif (core_sparse_checkout_cone)\n>>  \t\twrite_cone_to_file(fp, pl);\n>> @@ -356,11 +355,13 @@ static int write_patterns_and_update(struct pattern_list *pl)\n>>  \t\twrite_patterns_to_file(fp, pl);\n>>  \n>>  \tif (commit_lock_file(&lk))\n>> -\t\tdie_errno(_(\"unable to write %s\"), get_locked_file_path(&lk));\n>> +\t\tdie_errno(_(\"unable to write %s\"), sparse_filename);\n>\n> Note the difference between \"get_lock\" and \"get_locked\" in these two.\n> The first will mention the tempfile name, and the second the destination\n> filename (and sparse_filename is the latter).\n\nI did consider ... to write %s.%s\", sparse_filename, LOCK_SUFFIX)\nbut then thought that the final name is what is more relevant to the\nend user.  Yes, while correcting unrelated error, I shouldn't have\ntried to improve unrelated end-user experience ;-).\n\n> Will send v2 later tonight. Thanks, Patrick, for noticing the problem in\n> the first place.\n\nYeah, thanks, both.\n"}]}