Re: [PATCH] pack-objects.c: Initialize read mutex in cmd_pack_objects

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 18, 2019, 09:21 UTC
Message-ID: <CACsJy8CRQdzMTZU0Khno+r+kqrzu-iG5J_fKnPfVMWe55azD2g@mail.gmail.com>
In-Reply-To: <20190118022736.36832-1-phogg@novamoon.net>

On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg <phogg@novamoon.net> wrote:

Show 8 quoted lines

>
> ac77d0c37 ("pack-objects: shrink size field in struct object_entry",
> 2018-04-14) added an extra usage of read_lock/read_unlock in the newly
> introduced oe_get_size_slow for thread safety in parallel calls to
> try_delta(). Unfortunately oe_get_size_slow is also used in serial
> code, some of which is called before the first invocation of
> ll_find_deltas. As such the read mutex is not guaranteed to be
> initialized.

This must be the SIZE() macros in type_size_sort(), isn't it? I think we hit the same problem (use of uninitialized mutex) in this same code not long ago. I wonder if there's anyway we can reliably test and catch this.

> Resolve this by splitting off the read mutex initialization from
> init_threaded_search. Instead initialize (and clean up) the read
> mutex in cmd_pack_objects.

Maybe move the mutex to 'struct packing_data' and initialize it in prepare_packing_data(), so we centralize mutex at two locations: generic ones go there, command-specific mutexes stay here in init_threaded_search(). We could also move oe_get_size_slow() back to pack-objects.c (the one outside builtin/).

Show 62 quoted lines

> Signed-off-by: Patrick Hogg <phogg@novamoon.net>
> ---
>  builtin/pack-objects.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 411aefd68..9084bef02 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2381,22 +2381,30 @@ static pthread_cond_t progress_cond;
>   */
>  static void init_threaded_search(void)
>  {
> -       init_recursive_mutex(&read_mutex);
>         pthread_mutex_init(&cache_mutex, NULL);
>         pthread_mutex_init(&progress_mutex, NULL);
>         pthread_cond_init(&progress_cond, NULL);
>         old_try_to_free_routine = set_try_to_free_routine(try_to_free_from_threads);
>  }
>
> +static void init_read_mutex(void)
> +{
> +       init_recursive_mutex(&read_mutex);
> +}
> +
>  static void cleanup_threaded_search(void)
>  {
>         set_try_to_free_routine(old_try_to_free_routine);
>         pthread_cond_destroy(&progress_cond);
> -       pthread_mutex_destroy(&read_mutex);
>         pthread_mutex_destroy(&cache_mutex);
>         pthread_mutex_destroy(&progress_mutex);
>  }
>
> +static void cleanup_read_mutex(void)
> +{
> +       pthread_mutex_destroy(&read_mutex);
> +}
> +
>  static void *threaded_find_deltas(void *arg)
>  {
>         struct thread_params *me = arg;
> @@ -3319,6 +3327,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>                 OPT_END(),
>         };
>
> +       init_read_mutex();
> +
>         if (DFS_NUM_STATES > (1 << OE_DFS_STATE_BITS))
>                 BUG("too many dfs states, increase OE_DFS_STATE_BITS");
>
> @@ -3495,5 +3505,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>                            _("Total %"PRIu32" (delta %"PRIu32"),"
>                              " reused %"PRIu32" (delta %"PRIu32")"),
>                            written, written_delta, reused, reused_delta);
> +
> +       cleanup_read_mutex();
>         return 0;
>  }
> --
> 2.20.1.windows.1
>

-- 
Duy