patch, 2 partsrefs/files-backend: fix two subtle error-handling bugs

This series fixes two bugs around the error-handling of writing packed-refs during ref deletion. The first one came from a real-world case. The second is impossible to trigger (currently), but since I noticed it while hunting the first one, I figured it was worth fixing.

  [1/2]: refs/files-backend: handle packed transaction prepare failure
  [2/2]: refs/files-backend: don't look at an aborted transaction

 refs/files-backend.c         | 16 +++++++++++++++-
 t/t1404-update-ref-errors.sh | 16 ++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

-Peff

In files_transaction_prepare(), if we have to delete some refs, we use a subordinate packed_transaction to do so. It's rare for that sub-transaction's prepare step to fail, since we hold the packed-refs lock. But if it does, we trigger a BUG() due to these steps:

  - we've attached the packed transaction to the files transaction as
    backend_data->packed_transaction

  - when the prepare step fails, the packed transaction cleans itself
    up, putting itself into the CLOSED state

  - the error value from preparing the packed transaction lets us know
    in files_transaction_prepare() that we should also clean up and
    return an error. We call files_transaction_cleanup(), which tries to
    abort backend_data->packed_transaction. Since it's already CLOSED,
    that triggers an assertion in ref_transaction_abort().

We can fix that by disconnecting the packed transaction from the outer files transaction, and then free-ing (not aborting!) it ourselves.

A few other options/alternatives I considered:

  - we could just make it a noop to abort a CLOSED transaction. But that
    seems less safe, since clearly this code expects (and enforces) a
    particular set of state transitions.

  - we could have files_transaction_cleanup() selectively call abort()
    vs free() based on the state of the on the packed transaction.
    That's basically a more restricted version of the above, but also
    potentially unsafe.

  - instead of disconnecting backend_data->packed_transaction on error,
    we could wait to install it until we successfully prepare. That
    might make the flow a little simpler, but it introduces a hassle.
    Earlier parts of files_transaction_prepare() that encounter an error
    will jump to the cleanup label, and expect that cleaning up the
    outer transaction will clean up the packed transaction, too. We'd
    have to adjust those sites to clean up the packed transaction.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c         | 10 ++++++++++
 t/t1404-update-ref-errors.sh | 16 ++++++++++++++++
 2 files changed, 26 insertions(+)

On Thu, Mar 21, 2019 at 05:28:44AM -0400, Jeff King wrote:

This actually isn't too bad. Here's what it would look like as a cleanup patch on top. I dunno if it's worth it or not.

-- >8 --
Subject: [PATCH] refs/files-backend: delay setting packed_transaction

When preparing a files_transaction, we have two pointers to the subordinate packed transaction: a local variable, and one in backend_data which will be carried forward if we succeed.

These always point to the same thing, so one is basically an alias of the other. But in some of the trickier cleanup code added by the last few commits, we have to set them to NULL if we've already freed the struct. We only _need_ to do this for the one in backend_data, but that leaves the local variable as a dangling pointer.

Instead, let's make the rules more obvious:

  - only point backend_data at the packed transaction when we know it is
    needed and preparation has succeeded. We should never need to roll
    back backend_data->packed_transaction

  - clean up the local packed_transaction variable on failure manually,
    instead of relying on files_transaction_cleanup() to do it

An alternative would be to drop the local variable entirely, and just use backend_data->packed_transaction. That works, but the resulting code is a bit harder to read because of the length of the name.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

On Thu, Mar 21, 2019 at 08:06:01PM -0400, Jeff King wrote:

I am quite glad that you tried this out, since I was curious to see how it would look when you mentioned it to Michael. While I think it can often be convenient to have a local variable sharing the address of some other pointer within a struct, I find the mixed usage here somewhat confusing.

So, I think that this patch is worthwhile, but I think you should introduce _this_ as 1/3, and then the existing 1/2 and 2/2 would become 2/3 and 3/3, respectively.

Introducing this as 1/3 means that you don't have to introduce changes that immediately have the variables mentioned in them renamed in a subsequent commit. I'm not sure which you feel is preferable to you, though.

Thanks, Taylor

When deleting refs, we hold packed-refs.lock and prepare a packed transaction to drop the refs from the packed-refs file. If it turns out that we don't need to rewrite the packed refs (e.g., because none of the deletions were present in the file), then we abort the transaction.

If that abort succeeds, then the transaction struct will have been freed, and we set our local pointer to NULL so we don't look at it again.

However, if it fails, then the struct will _still_ have been freed (because ref_transaction_abort() always frees). But we don't clean up the pointer, and will jump to our cleanup code, which will try to abort it again, causing a use-after-free.

It's actually impossible for this to trigger in practice, since packed_transaction_abort() will never return anything but success. But let's fix it anyway, since that's more than we should assume about the packed-refs code (after all, we are already bothering to check for an error result which cannot be triggered).

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)