I was apprehensive about this from the very beginning. Anyway, for now I will be dropping this issue from the series.
On Tue, Nov 7, 2023 at 10:11 PM Laszlo Ersek <ler...@redhat.com> wrote: > On 11/7/23 07:19, Ranbir Singh wrote: > > From: Ranbir Singh <ranbir.sin...@dell.com> > > > > The function PciHostBridgeResourceAllocator is not making use of the > > generic approach as is used in one of the other function namely - > > DumpResourceMap. As a result, the following warnings can be seen as > > reported by Coverity e.g. > > > > (30) Event address_of: Taking address with "&IoBridge" yields a > > singleton pointer. > > (31) Event callee_ptr_arith: Passing "&IoBridge" to function > > "FindResourceNode" which uses it as an array. This might corrupt > > or misinterpret adjacent memory locations. > > > > Hence, adopt the generic approach to fix the issues at relevant points. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > > > > Cc: Ray Ni <ray...@intel.com> > > Co-authored-by: Veeresh Sangolli <veeresh.sango...@dellteam.com> > > Signed-off-by: Ranbir Singh <ranbir.sin...@dell.com> > > Signed-off-by: Ranbir Singh <rsi...@ventanamicro.com> > > --- > > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 37 ++++++++++++++++---- > > 1 file changed, 31 insertions(+), 6 deletions(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > index 84fc0161a19c..71767d3793d4 100644 > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > > @@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator ( > > UINT64 Mem64ResStatus; > > UINT64 PMem64ResStatus; > > UINT32 MaxOptionRomSize; > > + PCI_RESOURCE_NODE **ChildResources; > > + UINTN ChildResourceCount; > > PCI_RESOURCE_NODE *IoBridge; > > PCI_RESOURCE_NODE *Mem32Bridge; > > PCI_RESOURCE_NODE *PMem32Bridge; > > @@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator ( > > // Create the entire system resource map from the information > collected by > > // enumerator. Several resource tree was created > > // > > - FindResourceNode (RootBridgeDev, &IoPool, &IoBridge); > > - FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge); > > - FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge); > > - FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge); > > - FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge); > > - > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]); > > + IoBridge = ChildResources[0]; > > ASSERT (IoBridge != NULL); > > + > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]); > > + Mem32Bridge = ChildResources[0]; > > ASSERT (Mem32Bridge != NULL); > > + > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]); > > + PMem32Bridge = ChildResources[0]; > > ASSERT (PMem32Bridge != NULL); > > + > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]); > > + Mem64Bridge = ChildResources[0]; > > ASSERT (Mem64Bridge != NULL); > > + > > + ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, > NULL); > > + ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > > + ASSERT (ChildResources != NULL); > > + FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]); > > + PMem64Bridge = ChildResources[0]; > > ASSERT (PMem64Bridge != NULL); > > > > // > > Sorry, but this is terrible. > > * First of all, Coverity is *wrong*. The C spec clearly states that, for > the purposes of pointer arithmetic, a singleton object behaves > identically to the first element of an array. > > So, immediately, the idea arises that we should just use > > PCI_RESOURCE_NODE *IoBridgeArray[1]; > > FindResourceNode (RootBridgeDev, &IoPool, IoBridgeArray) > > to shut up Coverity. > > * Unfortunately, I expect that would only create a different warning: a > warning about potentially overflowing this single-element array. Which > is in fact a deeper problem in FindResourceNode() -- it happily > overwrites an array that is too small. > > * Finally, this generic approach is both ugly (lots of code > duplication!), and worse, it allocates memory without proper error > checking (ASSERT() is not error checking), and then it leaks > ChildResources *multiple times*! > > I suggest the following, for solving all of these issues: > > - create a function called FindFirstResourceNode(). It should have the > same function prototype as FindResourceNode(), except the last parameter > should be mandatory, not OPTIONAL. > > - the internal logic should be the same, except we shouldn't be > counting, the return value should be a BOOLEAN. If we find a match, then > output the first match and return TRUE. Otherwise, set the output to > NULL and return FALSE. > > - replace the FindResourceNode calls with FindFirstResourceNode calls > > - Those call sites are already followed by ASSERT()s, saying that all > search attempts will succeed. If coverity is happy with them, that's > good; otherwise, we'll have to find a solution for them as well. > > Laszlo > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111016): https://edk2.groups.io/g/devel/message/111016 Mute This Topic: https://groups.io/mt/102438300/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
