From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5F7EF211B76AE for ; Tue, 15 Jan 2019 02:29:48 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DC0B958E33; Tue, 15 Jan 2019 10:29:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-50.rdu2.redhat.com [10.10.120.50]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5466960A97; Tue, 15 Jan 2019 10:29:46 +0000 (UTC) To: Jiaxin Wu , edk2-devel@lists.01.org Cc: Wu Hao A , Ye Ting , Fu Siyuan , Gao Liming References: <20190115012613.16748-1-Jiaxin.wu@intel.com> <20190115012613.16748-2-Jiaxin.wu@intel.com> From: Laszlo Ersek Message-ID: <953600c3-5346-3d17-cdaa-9574c3d94d14@redhat.com> Date: Tue, 15 Jan 2019 11:29:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190115012613.16748-2-Jiaxin.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 15 Jan 2019 10:29:48 +0000 (UTC) Subject: Re: [PATCH v1 1/2] MdeModulePkg/NetLib.h: Fix the possible NULL pointer dereference issue. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Jan 2019 10:29:48 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Jiaxin, On 01/15/19 02:26, Jiaxin Wu wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1456 > > For the NET_LIST_FOR_EACH & NET_LIST_FOR_EACH_SAFE, "Entry" should be > checked whether it's NULL or not instead of using the pointer directly. > > Cc: Wu Hao A > Cc: Gao Liming > Cc: Ye Ting > Cc: Fu Siyuan > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Wu Jiaxin > --- > MdeModulePkg/Include/Library/NetLib.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/MdeModulePkg/Include/Library/NetLib.h b/MdeModulePkg/Include/Library/NetLib.h > index 0977973921..5b1307553a 100644 > --- a/MdeModulePkg/Include/Library/NetLib.h > +++ b/MdeModulePkg/Include/Library/NetLib.h > @@ -616,21 +616,21 @@ NetRandomInitSeed ( > > // > // Iterate through the double linked list. It is NOT delete safe > // > #define NET_LIST_FOR_EACH(Entry, ListHead) \ > - for(Entry = (ListHead)->ForwardLink; Entry != (ListHead); Entry = Entry->ForwardLink) > + for(Entry = (ListHead)->ForwardLink; Entry != (ListHead), Entry != NULL; Entry = Entry->ForwardLink) (1) The linked list from BaseLib (LIST_ENTRY) always has at least one element (the head element), and the list is empty if the head element points back to itself. In other words, ForwardLink may never be NULL. So why is it necessary to check against that case here? (2) If the NULL check is indeed necessary for some reason, then we should write Entry != (ListHead) && Entry != NULL in the controlling expression. Because, with the comma operator, the (Entry != (ListHead)) expression would be evaluated, but its result would be ignored. Thanks, Laszlo > > // > // Iterate through the double linked list. This is delete-safe. > // Don't touch NextEntry. Also, don't use this macro if list > // entries other than the Entry may be deleted when processing > // the current Entry. > // > #define NET_LIST_FOR_EACH_SAFE(Entry, NextEntry, ListHead) \ > - for(Entry = (ListHead)->ForwardLink, NextEntry = Entry->ForwardLink; \ > - Entry != (ListHead); \ > + for(Entry = (ListHead)->ForwardLink, (Entry != NULL) ? (NextEntry = Entry->ForwardLink) : (Entry = NULL); \ > + Entry != (ListHead), Entry != NULL; \ > Entry = NextEntry, NextEntry = Entry->ForwardLink \ > ) > > // > // Make sure the list isn't empty before getting the first/last record. >