Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <30aabd24-2b5f-5f5f-9bdd-0c505cd37b6d@redhat.com>
Date:   Tue, 1 Aug 2023 10:01:06 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: Double-Free and Memory Leak Found In libtirpc
Content-Language: en-US
To:     "Wartens, Herb" <wartens2@llnl.gov>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
References: <E931E05A-A78D-4802-9877-B04E9F610817@llnl.gov>
 <968E8957-2AF6-4901-84B0-92EDB5791131@llnl.gov>
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <968E8957-2AF6-4901-84B0-92EDB5791131@llnl.gov>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 7/27/23 5:48 PM, Wartens, Herb wrote:
> 
> Just to be clear...
> These patches were not made for the upstream branch (might not apply as cleanly as expected). I applied and tested them against libtirpc-1.1.4-8.el8. I have not gone through the trouble of verifying/testing them against upstream sources. Was just asked to mail these patches here by RH in the bug. Hopefully this is still helpful.
Thank you! I'm looking into them now...

steved.
> 
> -Herb
> 
> 
>> On Jul 27, 2023, at 9:08 AM, Wartens, Herb <wartens2@llnl.gov> wrote:
>>
>> Hello All,
>> We have opened up two separate RedHat bugs for these issues. I added patches to those bugs, but was asked to send the patches here as well since the patches might need to go upstream first.
>>
>> 1) https://bugzilla.redhat.com/show_bug.cgi?id=2224666
>>
>> We have an application called HPSS that heavily uses libtirpc. When we updated to RHEL8.8 our application started crashing all of a sudden. We believe the change that introduced this problem was 2112116:
>>
>> 2022-08-03 Steve Dickson mailto:steved@redhat.com 1.1.4-8
>> - rpcb_clnt.c add mechanism to try v2 protocol first (bz 2107650)
>> - Multithreaded cleanup (bz 2112116)
>>
>> 252     for (cptr = front; cptr != NULL; cptr = cptr->ac_next) {
>> 253         if (!memcmp(cptr->ac_taddr->buf, addr->buf, addr->len)) {
>> 254             /* Unlink from cache. We'll destroy it after releasing the mutex. */
>> 255             if (cptr->ac_uaddr)
>> 256                 free(cptr->ac_uaddr);
>> 257             if (prevptr)
>> 258                 prevptr->ac_next = cptr->ac_next;
>> 259             else
>> 260                 front = cptr->ac_next;
>> 261             cachesize--;
>> 262             break;
>> 263         }
>> 264         prevptr = cptr;
>> 265     }
>> 266
>> 267     mutex_unlock(&rpcbaddr_cache_lock);
>> 268     destroy_addr(cptr);
>>
>> so we have free'd cptr->ac_uaddr. I believe after that free probably safer to set cptr->ac_uaddr to NULL.
>> Note that destroy_addr() will also try to free it.
>>
>> 2) https://bugzilla.redhat.com/show_bug.cgi?id=2225226
>>
>> While inspecting the changes between the versions of libtirpc in question, I noticed a memory leak as well.
>>
>> /*
>> + * Destroys a cached address entry structure.
>> + *
>> + */
>> +static void
>> +destroy_addr(addr)
>> +       struct address_cache *addr;
>> +{
>> +       if (addr == NULL)
>> +               return;
>> +       if(addr->ac_host != NULL)
>> +               free(addr->ac_host);
>> +       if(addr->ac_netid != NULL)
>> +               free(addr->ac_netid);
>> +       if(addr->ac_uaddr != NULL)
>> +               free(addr->ac_uaddr);
>> +       if(addr->ac_taddr != NULL) {
>> +               if(addr->ac_taddr->buf != NULL)
>> +                       free(addr->ac_taddr->buf);
>> +       }
>> +       free(addr);
>> +}
>>
>> Pretty clear that addr->ac_taddr never was properly free’d. I also verified that with valgrind.
>>
>> I am happy to add more detail, but hopefully others on this list can access the bugs in question. If not let me know and I can add more detail here if needed. Thanks.
>>
>> -Herb
>>
>> <libtirpc-1.1.4-LLNL-memleak.patch><libtirpc-1.1.4-LLNL-crash.patch>
>