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

[Ammar Nofan Faizi <ammar_nofan_faizi@yahoo.com>]

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

-- 
Ammar Faizi