From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.85.128.68, mailfrom: philmd@redhat.com) Received: from mail-wm1-f68.google.com (mail-wm1-f68.google.com [209.85.128.68]) by groups.io with SMTP; Mon, 15 Apr 2019 10:31:58 -0700 Received: by mail-wm1-f68.google.com with SMTP id a184so21684513wma.2 for ; Mon, 15 Apr 2019 10:31:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:openpgp:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=1fb7UZlICepwxTFfGuW+K2r4saRShphWHE721JVVb00=; b=aWKT4S2Hl1Yg/ol1v+dW8TjVxJhDPQZ5QUB1K1I4Od+72lXjhSyqcjQXOjySlCDNCJ HLlHRw43cDWi5M5V1eL6iSGai3zgaTmwgH+gooobpsDJ9WjcJiXuAd1nLmvAdPsZVlr3 EDsfPjSmztRj403pqotWiikTAJfftIwm9ue5O9xXen+xsmyZ2/r2QA6NM1lS4ErfzpCM wgh5r812ALRVjH1Sk0yzS8NaHo2xq/3NMBdZpaPtw3wvzpgsPGaItyueVsv3t9g/csWo s3aQdJAQ1HwopKuqJ/5FjGPUtvf30q/gqqRivp223x+2MI1pbJK7jp5E/a4GynNyNoip 6uuA== X-Gm-Message-State: APjAAAVfnOa2sH3qCE0zHdLMG7E+PxeI63HK4BBab4crumLM+8PaI6Ya bRp8eCYOfTTdnFas/SzAXYvjFg== X-Google-Smtp-Source: APXvYqz5QgYABma1TfSYuHxhi8mKpJ5a8fEHubSyWljBFjkTBGvkvAhkKHYZ3lRjCREtTJdHNPXLKg== X-Received: by 2002:a1c:be13:: with SMTP id o19mr22026604wmf.19.1555349516363; Mon, 15 Apr 2019 10:31:56 -0700 (PDT) Return-Path: Received: from [10.32.224.40] (red-hat-inc.vlan560.asr1.mad1.gblx.net. [159.63.51.90]) by smtp.gmail.com with ESMTPSA id a11sm14890259wmm.35.2019.04.15.10.31.55 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 15 Apr 2019 10:31:55 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 10/10] OvmfPkg/BasePciCapLib: suppress invalid "nullptr deref" warning To: devel@edk2.groups.io, lersek@redhat.com Cc: Ard Biesheuvel , Jordan Justen References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-11-lersek@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Openpgp: id=89C1E78F601EE86C867495CBA2A3FD6EDEADC0DE; url=http://pgp.mit.edu/pks/lookup?op=get&search=0xA2A3FD6EDEADC0DE Message-ID: <98bcdd29-6df6-b13e-77df-a59b07555d8d@redhat.com> Date: Mon, 15 Apr 2019 19:31:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190412233128.4756-11-lersek@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 4/13/19 1:31 AM, Laszlo Ersek wrote: > RH covscan reports the following "nullptr deref" warning: > >> Error: CLANG_WARNING: >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:312:5: >> warning: Dereference of null pointer >> # InstanceZero->NumInstancesUnion.NumInstances++; >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:509:7: >> note: Assuming 'OutCapList' is not equal to NULL >> # if (OutCapList == NULL) { >> # ^~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:509:3: >> note: Taking false branch >> # if (OutCapList == NULL) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:518:7: >> note: Assuming the condition is false >> # if (OutCapList->Capabilities == NULL) { >> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:518:3: >> note: Taking false branch >> # if (OutCapList->Capabilities == NULL) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:529:7: >> note: Assuming 'CapHdrOffsets' is not equal to NULL >> # if (CapHdrOffsets == NULL) { >> # ^~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:529:3: >> note: Taking false branch >> # if (CapHdrOffsets == NULL) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:546:3: >> note: Taking false branch >> # if (RETURN_ERROR (Status)) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:549:7: >> note: Assuming the condition is true >> # if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) { >> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:549:3: >> note: Taking true branch >> # if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 0) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:557:5: >> note: Taking false branch >> # if (RETURN_ERROR (Status)) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:565:12: >> note: Assuming 'NormalCapHdrOffset' is > 0 >> # while (NormalCapHdrOffset > 0) { >> # ^~~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:565:5: >> note: Loop condition is true. Entering loop body >> # while (NormalCapHdrOffset > 0) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:570:7: >> note: Taking false branch >> # if (RETURN_ERROR (Status)) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:574:16: >> note: Calling 'InsertPciCap' >> # Status = InsertPciCap (OutCapList, CapHdrOffsets, PciCapNormal, >> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:235:3: >> note: Null pointer value stored to 'InstanceZero' >> # InstanceZero = NULL; >> # ^~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:243:7: >> note: Assuming 'PciCap' is not equal to NULL >> # if (PciCap == NULL) { >> # ^~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:243:3: >> note: Taking false branch >> # if (PciCap == NULL) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:259:3: >> note: Taking false branch >> # if (RETURN_ERROR (Status)) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:297:3: >> note: Taking false branch >> # if (RETURN_ERROR (Status)) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:311:7: >> note: Assuming the condition is true >> # if (PciCap->Key.Instance > 0) { >> # ^~~~~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:311:3: >> note: Taking true branch >> # if (PciCap->Key.Instance > 0) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:312:5: >> note: Dereference of null pointer >> # InstanceZero->NumInstancesUnion.NumInstances++; >> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> # 310| // >> # 311| if (PciCap->Key.Instance > 0) { >> # 312|-> InstanceZero->NumInstancesUnion.NumInstances++; >> # 313| } >> # 314| return RETURN_SUCCESS; > > The warning is invalid: the flagged dereferencing of "InstanceZero" is > gated by a condition that is only satisfied if we dereference > "InstanceZero" *first*. > > (Perhaps the analyzer assumes that the OrderedCollectionInsert() call, > just before line 259, can change the value of "PciCap->Key.Instance" via > the last argument: > > 254 // > 255 // Add PciCap to CapList. > 256 // > 257 Status = OrderedCollectionInsert (CapList->Capabilities, &PciCapEntry, > 258 PciCap); > 259 if (RETURN_ERROR (Status)) { > > That assumption is incorrect.) > > Add a comment and an ASSERT(). > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710 > Issue: scan-0994.txt > Signed-off-by: Laszlo Ersek > --- > OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c > index 4968d2b478db..c6f2c726509f 100644 > --- a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c > +++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c > @@ -298,16 +298,23 @@ InsertPciCap ( > goto DeletePciCapFromCapList; > } > > // > // Now we can bump the instance count maintained in Instance#0, if PciCap is > // not the first instance of (Domain, CapId). > // > if (PciCap->Key.Instance > 0) { > + // > + // Suppress invalid "nullptr dereference" compiler/analyzer warnings: the > + // only way for "PciCap->Key.Instance" to be positive here is for it to > + // have been assigned *from* dereferencing "InstanceZero" above. > + // > + ASSERT (InstanceZero != NULL); What about adding a STATIC_ANALYZER_HINT() only defined by covscan? STATIC_ANALYZER_HINT (InstanceZero != NULL); Regardless: Reviewed-by: Philippe Mathieu-Daude > + > InstanceZero->NumInstancesUnion.NumInstances++; > } > return RETURN_SUCCESS; > > DeletePciCapFromCapList: > OrderedCollectionDelete (CapList->Capabilities, PciCapEntry, NULL); > > FreePciCap: >