From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8C3E5C433B4 for ; Wed, 28 Apr 2021 18:04:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 55CC761003 for ; Wed, 28 Apr 2021 18:04:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S243395AbhD1SFT (ORCPT ); Wed, 28 Apr 2021 14:05:19 -0400 Received: from smtp-bc0b.mail.infomaniak.ch ([45.157.188.11]:43615 "EHLO smtp-bc0b.mail.infomaniak.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241704AbhD1SFR (ORCPT ); Wed, 28 Apr 2021 14:05:17 -0400 Received: from smtp-3-0000.mail.infomaniak.ch (unknown [10.4.36.107]) by smtp-2-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4FVmjn5c3FzMqHN0; Wed, 28 Apr 2021 20:04:29 +0200 (CEST) Received: from ns3096276.ip-94-23-54.eu (unknown [23.97.221.149]) by smtp-3-0000.mail.infomaniak.ch (Postfix) with ESMTPA id 4FVmjm5KbLzlh8T6; Wed, 28 Apr 2021 20:04:28 +0200 (CEST) Subject: Re: [PATCH] samples/landlock: fix path_list memory leak To: Tom Rix , Nick Desaulniers Cc: Nathan Chancellor , linux-security-module@vger.kernel.org, LKML , clang-built-linux , James Morris , Linus Torvalds References: <20210427183755.2790654-1-trix@redhat.com> <6108e69b-0470-cd71-e477-ba64641cbf58@digikod.net> <4ece80d4-16fe-1938-7eba-2046840881f6@redhat.com> From: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= Message-ID: Date: Wed, 28 Apr 2021 20:04:57 +0200 User-Agent: MIME-Version: 1.0 In-Reply-To: <4ece80d4-16fe-1938-7eba-2046840881f6@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28/04/2021 17:36, Tom Rix wrote: > > On 4/28/21 2:58 AM, Mickaël Salaün wrote: >> On 27/04/2021 21:13, Nick Desaulniers wrote: >>> On Tue, Apr 27, 2021 at 11:38 AM wrote: >>>> From: Tom Rix >>>> >>>> Clang static analysis reports this error >>>> >>>> sandboxer.c:134:8: warning: Potential leak of memory >>>>    pointed to by 'path_list' >>>>          ret = 0; >>>>                ^ >>>> path_list is allocated in parse_path() but never freed. >>>> >>>> Signed-off-by: Tom Rix >>>> --- >>>>   samples/landlock/sandboxer.c | 2 ++ >>>>   1 file changed, 2 insertions(+) >>>> >>>> diff --git a/samples/landlock/sandboxer.c >>>> b/samples/landlock/sandboxer.c >>>> index 7a15910d2171..4629d011ed61 100644 >>>> --- a/samples/landlock/sandboxer.c >>>> +++ b/samples/landlock/sandboxer.c >>>> @@ -134,6 +134,8 @@ static int populate_ruleset( >>>>          ret = 0; >>>> >>>>   out_free_name: >>>> +       if (path_list) >>>> +               free(path_list); >>> I don't think the conditional is even necessary? By our first `goto >>> out_free_name;`, `parse_path` has already been called/memory for >>> `path_list` has already been allocated. `parse_path` doesn't check >>> whether `malloc` has failed. >> Indeed, no need for the path_list check. In practice, this memory leak >> doesn't stay long because of the execve, but I missed this free anyway. >> Thanks! > > Ok, the general problem of not checking if malloc and friends succeeds > is a different problem. > > So remove the check and keep the free ? Yes please. > > Tom > >> Reviewed-by: Mickaël Salaün >> >>>>          free(env_path_name); >>>>          return ret; >>>>   } >>>> -- >>>> 2.26.3 >>>> >>> >