{"thread":{"id":"50792","subject":"[PATCH 0/2] refs/files-backend: fix two subtle error-handling bugs","startedAt":"2019-03-21T09:28:32Z","lastAt":"2019-03-22T14:35:02Z","messageCount":5,"participants":["Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"372079","messageId":"20190321092829.GA2648@sigill.intra.peff.net","threadId":"50792","inReplyTo":null,"subject":"[PATCH 0/2] refs/files-backend: fix two subtle error-handling bugs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-21T09:28:29Z","receivedAt":"2019-03-21T09:28:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes two bugs around the error-handling of writing\npacked-refs during ref deletion. The first one came from a real-world\ncase.  The second is impossible to trigger (currently), but since I\nnoticed it while hunting the first one, I figured it was worth fixing.\n\n  [1/2]: refs/files-backend: handle packed transaction prepare failure\n  [2/2]: refs/files-backend: don't look at an aborted transaction\n\n refs/files-backend.c         | 16 +++++++++++++++-\n t/t1404-update-ref-errors.sh | 16 ++++++++++++++++\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\n-Peff\n"},{"id":"372080","messageId":"20190321092844.GA2722@sigill.intra.peff.net","threadId":"50792","inReplyTo":"20190321092829.GA2648@sigill.intra.peff.net","subject":"[PATCH 1/2] refs/files-backend: handle packed transaction prepare failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-21T09:28:44Z","receivedAt":"2019-03-21T09:28:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In files_transaction_prepare(), if we have to delete some refs, we use a\nsubordinate packed_transaction to do so. It's rare for that\nsub-transaction's prepare step to fail, since we hold the packed-refs\nlock. But if it does, we trigger a BUG() due to these steps:\n\n  - we've attached the packed transaction to the files transaction as\n    backend_data->packed_transaction\n\n  - when the prepare step fails, the packed transaction cleans itself\n    up, putting itself into the CLOSED state\n\n  - the error value from preparing the packed transaction lets us know\n    in files_transaction_prepare() that we should also clean up and\n    return an error. We call files_transaction_cleanup(), which tries to\n    abort backend_data->packed_transaction. Since it's already CLOSED,\n    that triggers an assertion in ref_transaction_abort().\n\nWe can fix that by disconnecting the packed transaction from the outer\nfiles transaction, and then free-ing (not aborting!) it ourselves.\n\nA few other options/alternatives I considered:\n\n  - we could just make it a noop to abort a CLOSED transaction. But that\n    seems less safe, since clearly this code expects (and enforces) a\n    particular set of state transitions.\n\n  - we could have files_transaction_cleanup() selectively call abort()\n    vs free() based on the state of the on the packed transaction.\n    That's basically a more restricted version of the above, but also\n    potentially unsafe.\n\n  - instead of disconnecting backend_data->packed_transaction on error,\n    we could wait to install it until we successfully prepare. That\n    might make the flow a little simpler, but it introduces a hassle.\n    Earlier parts of files_transaction_prepare() that encounter an error\n    will jump to the cleanup label, and expect that cleaning up the\n    outer transaction will clean up the packed transaction, too. We'd\n    have to adjust those sites to clean up the packed transaction.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c         | 10 ++++++++++\n t/t1404-update-ref-errors.sh | 16 ++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex ef053f716c..2186cbbe26 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2695,6 +2695,16 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tif (is_packed_transaction_needed(refs->packed_ref_store,\n \t\t\t\t\t\t packed_transaction)) {\n \t\t\tret = ref_transaction_prepare(packed_transaction, err);\n+\t\t\t/*\n+\t\t\t * A failure during the prepare step will abort\n+\t\t\t * itself, but not free. Do that now, and disconnect\n+\t\t\t * from the files_transaction so it does not try to\n+\t\t\t * abort us when we hit the cleanup code below.\n+\t\t\t */\n+\t\t\tif (ret) {\n+\t\t\t\tref_transaction_free(packed_transaction);\n+\t\t\t\tbackend_data->packed_transaction = NULL;\n+\t\t\t}\n \t\t} else {\n \t\t\t/*\n \t\t\t * We can skip rewriting the `packed-refs`\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 6b6a8e2292..970c5c36b9 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -618,4 +618,20 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' '\n \ttest_cmp unchanged actual\n '\n \n+test_expect_success 'delete fails cleanly if packed-refs.new write fails' '\n+\t# Setup and expectations are similar to the test above.\n+\tprefix=refs/failed-packed-refs &&\n+\tgit update-ref $prefix/foo $C &&\n+\tgit pack-refs --all &&\n+\tgit update-ref $prefix/foo $D &&\n+\tgit for-each-ref $prefix >unchanged &&\n+\t# This should not happen in practice, but it is an easy way to get a\n+\t# reliable error (we open with create_tempfile(), which uses O_EXCL).\n+\t: >.git/packed-refs.new &&\n+\ttest_when_finished \"rm -f .git/packed-refs.new\" &&\n+\ttest_must_fail git update-ref -d $prefix/foo &&\n+\tgit for-each-ref $prefix >actual &&\n+\ttest_cmp unchanged actual\n+'\n+\n test_done\n-- \n2.21.0.701.g4401309e11\n\n"},{"id":"372081","messageId":"20190321092853.GB2722@sigill.intra.peff.net","threadId":"50792","inReplyTo":"20190321092829.GA2648@sigill.intra.peff.net","subject":"[PATCH 2/2] refs/files-backend: don't look at an aborted transaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-21T09:28:54Z","receivedAt":"2019-03-21T09:28:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When deleting refs, we hold packed-refs.lock and prepare a packed\ntransaction to drop the refs from the packed-refs file. If it turns out\nthat we don't need to rewrite the packed refs (e.g., because none of the\ndeletions were present in the file), then we abort the transaction.\n\nIf that abort succeeds, then the transaction struct will have been\nfreed, and we set our local pointer to NULL so we don't look at it\nagain.\n\nHowever, if it fails, then the struct will _still_ have been freed\n(because ref_transaction_abort() always frees). But we don't clean up\nthe pointer, and will jump to our cleanup code, which will try to abort\nit again, causing a use-after-free.\n\nIt's actually impossible for this to trigger in practice, since\npacked_transaction_abort() will never return anything but success. But\nlet's fix it anyway, since that's more than we should assume about the\npacked-refs code (after all, we are already bothering to check for an\nerror result which cannot be triggered).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2186cbbe26..442204af4d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2711,12 +2711,16 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t * file. But we do need to leave it locked, so\n \t\t\t * that somebody else doesn't pack a reference\n \t\t\t * that we are trying to delete.\n+\t\t\t *\n+\t\t\t * We need to disconnect our transaction from\n+\t\t\t * backend_data, since the abort (whether successful or\n+\t\t\t * not) will free it.\n \t\t\t */\n+\t\t\tbackend_data->packed_transaction = NULL;\n \t\t\tif (ref_transaction_abort(packed_transaction, err)) {\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n-\t\t\tbackend_data->packed_transaction = NULL;\n \t\t}\n \t}\n \n-- \n2.21.0.701.g4401309e11\n"},{"id":"372192","messageId":"20190322000601.GA32671@sigill.intra.peff.net","threadId":"50792","inReplyTo":"20190321092844.GA2722@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] refs/files-backend: handle packed transaction prepare failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-22T00:06:01Z","receivedAt":"2019-03-22T00:06:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 21, 2019 at 05:28:44AM -0400, Jeff King wrote:\n\n>   - instead of disconnecting backend_data->packed_transaction on error,\n>     we could wait to install it until we successfully prepare. That\n>     might make the flow a little simpler, but it introduces a hassle.\n>     Earlier parts of files_transaction_prepare() that encounter an error\n>     will jump to the cleanup label, and expect that cleaning up the\n>     outer transaction will clean up the packed transaction, too. We'd\n>     have to adjust those sites to clean up the packed transaction.\n\nThis actually isn't too bad. Here's what it would look like as a\ncleanup patch on top. I dunno if it's worth it or not.\n\n-- >8 --\nSubject: [PATCH] refs/files-backend: delay setting packed_transaction\n\nWhen preparing a files_transaction, we have two pointers to the\nsubordinate packed transaction: a local variable, and one in\nbackend_data which will be carried forward if we succeed.\n\nThese always point to the same thing, so one is basically an alias of\nthe other. But in some of the trickier cleanup code added by the last\nfew commits, we have to set them to NULL if we've already freed the\nstruct. We only _need_ to do this for the one in backend_data, but that\nleaves the local variable as a dangling pointer.\n\nInstead, let's make the rules more obvious:\n\n  - only point backend_data at the packed transaction when we know it is\n    needed and preparation has succeeded. We should never need to roll\n    back backend_data->packed_transaction\n\n  - clean up the local packed_transaction variable on failure manually,\n    instead of relying on files_transaction_cleanup() to do it\n\nAn alternative would be to drop the local variable entirely, and just\nuse backend_data->packed_transaction. That works, but the resulting code\nis a bit harder to read because of the length of the name.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c | 41 ++++++++++++++++++++++++-----------------\n 1 file changed, 24 insertions(+), 17 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 442204af4d..13a53bcb30 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2672,9 +2672,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n \t\t\t\t}\n-\n-\t\t\t\tbackend_data->packed_transaction =\n-\t\t\t\t\tpacked_transaction;\n \t\t\t}\n \n \t\t\tref_transaction_add_update(\n@@ -2695,15 +2692,23 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tif (is_packed_transaction_needed(refs->packed_ref_store,\n \t\t\t\t\t\t packed_transaction)) {\n \t\t\tret = ref_transaction_prepare(packed_transaction, err);\n-\t\t\t/*\n-\t\t\t * A failure during the prepare step will abort\n-\t\t\t * itself, but not free. Do that now, and disconnect\n-\t\t\t * from the files_transaction so it does not try to\n-\t\t\t * abort us when we hit the cleanup code below.\n-\t\t\t */\n-\t\t\tif (ret) {\n+\t\t\tif (!ret) {\n+\t\t\t\t/*\n+\t\t\t\t * Attach the prepared packed transaction to\n+\t\t\t\t * the files transaction so it can be committed\n+\t\t\t\t * later.\n+\t\t\t\t */\n+\t\t\t\tbackend_data->packed_transaction =\n+\t\t\t\t\tpacked_transaction;\n+\t\t\t} else {\n+\t\t\t\t/*\n+\t\t\t\t * A failure during the prepare step will abort\n+\t\t\t\t * itself, but not free. Do that now, so we\n+\t\t\t\t * don't try to double-abort during the cleanup\n+\t\t\t\t * below.\n+\t\t\t\t */\n \t\t\t\tref_transaction_free(packed_transaction);\n-\t\t\t\tbackend_data->packed_transaction = NULL;\n+\t\t\t\tpacked_transaction = NULL;\n \t\t\t}\n \t\t} else {\n \t\t\t/*\n@@ -2712,25 +2717,27 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t * that somebody else doesn't pack a reference\n \t\t\t * that we are trying to delete.\n \t\t\t *\n-\t\t\t * We need to disconnect our transaction from\n-\t\t\t * backend_data, since the abort (whether successful or\n-\t\t\t * not) will free it.\n+\t\t\t * We need to NULL our local pointer, since the abort\n+\t\t\t * (whether successful or not) will free it.\n \t\t\t */\n-\t\t\tbackend_data->packed_transaction = NULL;\n \t\t\tif (ref_transaction_abort(packed_transaction, err)) {\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tpacked_transaction = NULL;\n \t\t\t\tgoto cleanup;\n \t\t\t}\n+\t\t\tpacked_transaction = NULL;\n \t\t}\n \t}\n \n cleanup:\n \tfree(head_ref);\n \tstring_list_clear(&affected_refnames, 0);\n \n-\tif (ret)\n+\tif (ret) {\n \t\tfiles_transaction_cleanup(refs, transaction);\n-\telse\n+\t\tif (packed_transaction)\n+\t\t\tref_transaction_abort(packed_transaction, err);\n+\t} else\n \t\ttransaction->state = REF_TRANSACTION_PREPARED;\n \n \treturn ret;\n-- \n2.21.0.705.g64cb90f133\n\n"},{"id":"372247","messageId":"20190322143457.GA14595@Taylors-MacBook-Pro.local","threadId":"50792","inReplyTo":"20190322000601.GA32671@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] refs/files-backend: handle packed transaction prepare failure","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-03-22T14:34:57Z","receivedAt":"2019-03-22T14:35:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 21, 2019 at 08:06:01PM -0400, Jeff King wrote:\n> On Thu, Mar 21, 2019 at 05:28:44AM -0400, Jeff King wrote:\n>\n> >   - instead of disconnecting backend_data->packed_transaction on error,\n> >     we could wait to install it until we successfully prepare. That\n> >     might make the flow a little simpler, but it introduces a hassle.\n> >     Earlier parts of files_transaction_prepare() that encounter an error\n> >     will jump to the cleanup label, and expect that cleaning up the\n> >     outer transaction will clean up the packed transaction, too. We'd\n> >     have to adjust those sites to clean up the packed transaction.\n>\n> This actually isn't too bad. Here's what it would look like as a\n> cleanup patch on top. I dunno if it's worth it or not.\n\nI am quite glad that you tried this out, since I was curious to see how\nit would look when you mentioned it to Michael. While I think it can\noften be convenient to have a local variable sharing the address of some\nother pointer within a struct, I find the mixed usage here somewhat\nconfusing.\n\nSo, I think that this patch is worthwhile, but I think you should\nintroduce _this_ as 1/3, and then the existing 1/2 and 2/2 would become\n2/3 and 3/3, respectively.\n\nIntroducing this as 1/3 means that you don't have to introduce changes\nthat immediately have the variables mentioned in them renamed in a\nsubsequent commit. I'm not sure which you feel is preferable to you,\nthough.\n\n> -- >8 --\n>\n> [ ... ]\n\nThanks,\nTaylor\n"}]}