Re: [PATCH] pack-bitmaps: plug memory leak, fix allocation size for recent_bitmaps

From: Stefan Beller <sbeller@google.com>
Date: May 19, 2015, 00:57 UTC
Message-ID: <CAGZ79kZeX_VqcvTt_kgGKyWH0SV2zsV_j1KuxNhwR18zWfp5LA@mail.gmail.com>
In-Reply-To: <555A7499.7090900@web.de>

On Mon, May 18, 2015 at 4:24 PM, René Scharfe <l.s.r@web.de> wrote:

Show 15 quoted lines

> Use an automatic variable for recent_bitmaps, an array of pointers.
> This way we don't allocate too much and don't have to free the memory
> at the end.  The old code over-allocated because it reserved enough
> memory to store all of the structs it is only pointing to and never
> freed it.  160 64-bit pointers take up 1280 bytes, which is not too
> much to be placed on the stack.
>
> MAX_XOR_OFFSET is turned into a preprocessor constant to make it
> constant enough for use in an non-variable array declaration.
>
> Noticed-by: Stefan Beller <stefanbeller@gmail.com>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> This seems to have fallen through the cracks, or did I just miss it?

I planned on resending it eventually. ;) But with the usual definition of 'eventually', it may take a while.

Thanks for picking this up! Stefan

Show 17 quoted lines

>
>  pack-bitmap.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 62a98cc..e5abb8a 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -209,14 +209,12 @@ static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos)
>         return buffer[(*pos)++];
>  }
>
> +#define MAX_XOR_OFFSET 160
> +
>  static int load_bitmap_entries_v1(struct bitmap_index *index)
>  {
> -       static const size_t MAX_XOR_OFFSET = 160;

Is there a reason why we prefer defines over a  static const size_t here?

Show 6 quoted lines

> -
>         uint32_t i;
> -       struct stored_bitmap **recent_bitmaps;
> -
> -       recent_bitmaps = xcalloc(MAX_XOR_OFFSET, sizeof(struct stored_bitmap));
> +       struct stored_bitmap *recent_bitmaps[MAX_XOR_OFFSET] = { NULL };

This looks fine to me.

Show 10 quoted lines

>
>         for (i = 0; i < index->entry_count; ++i) {
>                 int xor_offset, flags;
> --
> 2.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html