Re: [PATCH 2/7] move setting of object->type to alloc_* functions
- From
Ramsay Jones <ramsay@ramsay1.demon.co.uk>
- Date
- Jul 12, 2014, 14:44 UTC
- Message-ID
- <53C149B6.7010705@ramsay1.demon.co.uk>
- In-Reply-To
- <20140711084611.GB5625@sigill.intra.peff.net>
On 11/07/14 09:46, Jeff King wrote:
Show 57 quoted lines
> The "struct object" type implements basic object
> polymorphism. Individual instances are allocated as
> concrete types (or as a union type that can store any
> object), and a "struct object *" can be cast into its real
> type after examining its "type" enum. This means it is
> dangerous to have a type field that does not match the
> allocation (e.g., setting the type field of a "struct blob"
> to "OBJ_COMMIT" would mean that a reader might read past the
> allocated memory).
>
> In most of the current code this is not a problem; the first
> thing we do after allocating an object is usually to set its
> type field by passing it to create_object. However, the
> virtual commits we create in merge-recursive.c do not ever
> get their type set. This does not seem to have caused
> problems in practice, though (presumably because we always
> pass around a "struct commit" pointer and never even look at
> the type).
>
> We can fix this oversight and also make it harder for future
> code to get it wrong by setting the type directly in the
> object allocation functions.
>
> This will also make it easier to fix problems with commit
> index allocation, as we know that any object allocated by
> alloc_commit_node will meet the invariant that an object
> with an OBJ_COMMIT type field will have a unique index
> number.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> alloc.c | 18 ++++++++++--------
> blob.c | 2 +-
> builtin/blame.c | 1 -
> commit.c | 2 +-
> object.c | 5 ++---
> object.h | 2 +-
> tag.c | 2 +-
> tree.c | 2 +-
> 8 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/alloc.c b/alloc.c
> index d7c3605..fd5fcb7 100644
> --- a/alloc.c
> +++ b/alloc.c
> @@ -18,11 +18,11 @@
>
> #define BLOCKING 1024
>
> -#define DEFINE_ALLOCATOR(name, type) \
> +#define DEFINE_ALLOCATOR(name, flag, type) \
> static struct alloc_state name##_state; \
> void *alloc_##name##_node(void) \
> { \
> - return alloc_node(&name##_state, sizeof(type)); \
> + return alloc_node(&name##_state, flag, sizeof(type)); \
> }I don't particularly like 'flag' here. (not a massive dislike, mind you:)
Perhaps: flag->object_type, type->node_type? Or, if that's too verbose, maybe just: flag->type, type->node?
ATB, Ramsay Jones