Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function

[Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>]

On Mon, Aug 29, 2022 at 11:54 AM Ammar Nofan Faizi wrote:
> On 8/29/22 11:41 AM, Alviro Iskandar Setiawan wrote:
>> On Mon, Aug 29, 2022 at 8:11 AM Ammar Faizi wrote:
>>> @@ -251,7 +254,7 @@ net::DefineNetworkTrafficAnnotation("CHNetDelegate", R"(
>>>          })");
>>>
>>>   CHNetDelegate::CHNetDelegate(void):
>>> -       thread_("chromium_thread"),
>>> +       thread_(*get_thread()),
>>>          method_("GET"),
>>>          err_("")
>>>   {
>>> @@ -287,6 +290,7 @@ CHNetDelegate::~CHNetDelegate(void)
>>>          r->PostTask(FROM_HERE, base::BindOnce(CHNetDelegateDestruct, &url_req_,
>>>                                                &url_req_ctx_, &sig));
>>>          sig.Wait();
>>> +       put_thread(&thread_);
>>>   }
>>
>> if @url_req_ and @url_req_ctx_ are both nullptr, this put_thread()
>> won't be called and we have a ref count leak
>
> Yes, you're right. Will fix it in the v2 revision.
>
>>>   template <typename T, typename... Types>
>>> @@ -629,6 +633,81 @@ static uint32_t g_max_ch_thpool;
>>>   static std::mutex g_thpool_lock_;
>>>   static struct ch_thpool **g_thpool;
>>>
>>> +
>>> +static base::Thread *get_thread(void)
>>> +{
>>> +       const uint32_t max_ch_thpool = g_max_ch_thpool;
>>> +       const uint32_t nr_ref_split = 2048;
>>> +       struct ch_thpool **thp;
>>> +       struct ch_thpool *ret = nullptr;
>>> +       struct ch_thpool *tmp;
>>> +       uint32_t min_ref_idx;
>>> +       uint32_t min_ref;
>>> +       uint32_t i;
>>> +
>>> +       g_thpool_lock_.lock();
>>> +       thp = g_thpool;
>>> +       if (!thp) {
>>> +               g_thpool_lock_.unlock();
>>> +               return nullptr;
>>> +       }
>>
>> in what situation @thp can be nullptr?
>
> When the chnet_global_destroy() is called.
>
>>> +       tmp = thp[0];
>>> +       if (!tmp) {
>>> +               ret = new struct ch_thpool;
>>> +               ret->idx_ = 0;
>>> +               thp[0] = ret;
>>> +               goto out;
>>> +       }
>>> +
>>> +       min_ref = tmp->ref_count_;
>>> +       min_ref_idx = 0;
>>> +       for (i = 1; i < max_ch_thpool; i++) {
>>> +               uint32_t ref;
>>> +
>>> +               tmp = thp[i];
>>> +               if (!tmp) {
>>> +                       ret = new struct ch_thpool;
>>> +                       ret->idx_ = i;
>>> +                       thp[i] = ret;
>>> +                       goto out;
>>> +               }
>>> +
>>> +               ref = tmp->ref_count_;
>>> +               if (ref < nr_ref_split) {
>>> +                       ret = tmp;
>>> +                       break;
>>> +               }
>>> +
>>> +               if (ref < min_ref) {
>>> +                       min_ref = ref;
>>> +                       min_ref_idx = i;
>>> +               }
>>> +       }
>>> +
>>> +       if (!ret)
>>> +               ret = thp[min_ref_idx];
>>> +
>>> +out:
>>> +       ret->ref_count_++;
>>> +       g_thpool_lock_.unlock();
>>> +       return &ret->thread_;
>>> +}
>>
>> this unlock() call will behave as a full memory barrier for that
>> @ref_count_ increment, is this really needed? you can have the
>> increment after unlock() tho
>
> No, the ref_count needs to be protected by a mutex. Otherwise, we
> have a use-after-free bug.
>
> Possible UAF scenario:
>
>        Thread1                Thread2
>        ----                   -------
> --> get_thread()
>      lock()
>      unlock()                 --> put_thread()
>      # preempted away         lock()
>                               decrement
>                               delete
>                               unlock()
>      increment # UAF!!!       return
>      return

ic ic, i understand now, i didn't see that coming

tq for explaining

tq

-- Viro