Re: [PATCH 02/11] treewide: prefer lockfiles on the stack

From: Jeff King <peff@peff.net>
Date: Oct 2, 2017, 05:34 UTC
Message-ID: <20171002053438.kpi3bhycqkgkg2o5@sigill.intra.peff.net>
In-Reply-To: <f241038eeed0f5f8bb346b394495356c20884a39.1506862824.git.martin.agren@gmail.com>

On Sun, Oct 01, 2017 at 04:56:03PM +0200, Martin Ågren wrote:

Show 10 quoted lines

> There is no longer any need to allocate and leak a `struct lock_file`.
> The previous patch addressed an instance where we needed a minor tweak
> alongside the trivial changes.
> 
> Deal with the remaining instances where we allocate and leak a struct
> within a single function. Change them to have the `struct lock_file` on
> the stack instead.
> 
> These instances were identified by running `git grep "^\s*struct
> lock_file\s*\*"`.

Thanks. These all look pretty straightforward, and as a general rule this _should_ be a safe conversion with respect to dangling references, since:

  - the tempfile pointers copied onto the global cleanup list are always
    allocated, so there's no danger of our stack variables going out of
    scope and leaving cruft in the global list.

  - when the lock is committed or rolled back, we NULL the pointer after
    freeing the tempfile. Nobody should be looking at the lock after
    that, but if there is such a bug, we should reliably hit an assert
    (if they use the accessors which check for validity) or a segfault
    (if somebody tries to dereference it directly).

    So it's possible that this subtly introduces a new segfault or
    assertion failure, but in that case it would be finding an
    _existing_ bug which was previously just reading cruft from the
    inactive tempfile struct.

-Peff