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

From: Patrick Hogg <phogg@novamoon.net>
Date: Jan 19, 2019, 01:46 UTC
Message-ID: <CAFOcBzmPAfq2ihF191KQAu0-_hz8DLdeAwDO-+gA-dR1sdND8Q@mail.gmail.com>
In-Reply-To: <CACsJy8A-kk9DEPZKWR26ye5tUHjny_18xf69CUevpRLJrs19PA@mail.gmail.com>

On Fri, Jan 18, 2019 at 8:10 AM Duy Nguyen <pclouds@gmail.com> wrote:

Show 29 quoted lines

>
> On Fri, Jan 18, 2019 at 8:04 PM Patrick Hogg <phogg@novamoon.net> wrote:
> >
> > On Fri, Jan 18, 2019 at 4:21 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >>
> >> On Fri, Jan 18, 2019 at 9:28 AM Patrick Hogg <phogg@novamoon.net> wrote:
> >> >
> >> > 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.
> >
> >
> > It was actually the SET_SIZE macro in check_object, at least for the repo at my company that hits this issue.  I took a look at the call tree for oe_get_size_slow and found that it's used in many places outside of ll_find_deltas, so there are many potential call sites where this could crop up:
> >
> >  [snip]
> >
>
> Ah, yes. I think the only problematic place is from prepare_pack().
> The single threaded access after ll_find_deltas() is fine because we
> never destroy mutexes.

I'm a bit confused, I see calls to pthread_mutex_destroy in cleanup_threaded_search. It's true that only prepare_packing_data(&to_pack) is called and there is no cleanup of the to_pack instance (at least as far as I can see) in cmd_pack_objects, but aren't the threaded_search mutexes destroyed?

Show 21 quoted lines

>
> > (Sorry if this is redundant for those who know the code better)
>
> Actually it's me to say sorry. I apparently did not know the code flow
> good enough to prevent this problem in the first place.
>
> >> > 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/).
> >
> >
> > I was already thinking that generic mutexes should be separated from command specific ones (that's why I introduced init_read_mutex and cleanup_read_mutex, but that may well not be the right exposure.)  I'll try my hand at this tonight (just moving the mutex to struct packing_data and initializing it in prepare_packing_data, I'll leave large code moves to the experts) and see how it turns out.
>
> Yes, leave the code move for now. Bug fixes stay small and simple (and
> get merged faster)

I was looking at this and noticed that packing_data already has a lock mutex member. Perhaps I am missing something but would it be appropriate to drop read_mutex altogether, change lock to be a recursive mutex, then use that instead in read_lock()/read_unlock()? (Or even to directly call packing_data_lock/packing_data_unlock instead of read_lock/read_unlock? Strictly speaking it would be a pack lock and not a read lock so the read_lock/read_unlock terminology wouldn't be accurate anymore.)

I have the change locally to move read_mutex to the packing_data struct (and rename it to read_lock to be consistent with the "lock" member), but it seems redundant. (And the lock member is only used in oe_set_delta_size.)

> --
> Duy