From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on gnuweeb.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NO_DNS_FOR_FROM,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6 Received: from mail-lf1-f54.google.com (mail-lf1-f54.google.com [209.85.167.54]) by gnuweeb.org (Postfix) with ESMTPSA id 05FDB80B33 for ; Mon, 29 Aug 2022 05:17:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gnuweeb.org; s=default; t=1661750256; bh=+YEUUFFi4pVgsaAt+rO47g0BYp1PlKRrOeZ7rMJdZys=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=c88Bz8haBLcHjplrXzK5ShQQGUDdovSDP9boam5qA3j0xhGQZcD+e7icmqqRoG1de +pXYuegiMfwBFNVOcvNSVBDf5pdYChjfo/5Kt7Kk35Y0X6NpQcrUZC/QelTtBjmL9+ jhB0QIXG3DvGaBKNNbB/UcZBncjqLWaGrQRp3uFE1qXBZvfC6JoO/77FMYJZ5QxK6M nTSalwMYGST7hxYzcdvEfxNVglaA/Hqm+77ollXAju4qKrVa5eFPGqTFGOFPcJJj+X 2RjX7CuIC0dvlN/Zaj1kwnOqiCSd6jvB8e+182P+U/n0IN+qMUfowpx/RhLk6it9QD Sj37gQV/iWEmg== Received: by mail-lf1-f54.google.com with SMTP id t23so4833581lfr.3 for ; Sun, 28 Aug 2022 22:17:35 -0700 (PDT) X-Gm-Message-State: ACgBeo24estpkgUSgyfuZdc8yXmAaAY4Qrh9JhmWZlEWno9c7KfcccZ0 w5v3CE/IM1Pnt5Err6eUsYxCGdS1jUhPi/OQeqU= X-Google-Smtp-Source: AA6agR4c6m+FErfad37/G8HuRfa9gioYoe9xOrzJyd+3NeTf22XtXdHZ6R4yzv01WUZ38Tk+hrA+4F7TRfbqSlIWtfY= X-Received: by 2002:a05:6512:3501:b0:48b:205f:91a2 with SMTP id h1-20020a056512350100b0048b205f91a2mr5698195lfs.83.1661750254105; Sun, 28 Aug 2022 22:17:34 -0700 (PDT) MIME-Version: 1.0 References: <20220829011127.3150320-1-ammarfaizi2@gnuweeb.org> <20220829011127.3150320-3-ammarfaizi2@gnuweeb.org> In-Reply-To: From: Alviro Iskandar Setiawan Date: Mon, 29 Aug 2022 12:17:22 +0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH v1 2/2] chnet: Implement `get_thread()` and `put_thread()` function To: Ammar Nofan Faizi Cc: Ammar Faizi , Muhammad Rizki , Kanna Scarlet , "GNU/Weeb Mailing List" Content-Type: text/plain; charset="UTF-8" List-Id: 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 >>> @@ -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