From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id F3220740032 for ; Tue, 27 Feb 2024 06:39:08 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=3pnNubcrUjZNB4CPovzl9zHFdcJOHYxvHXjxTeCW5Pw=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:From:To:Cc:Reply-To:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1709015947; v=1; b=JaxPxsPPZLjVjn6yVN68Y5mIFyfCJFCaG2GoyuWAFu+D/swnadofmj3GiQwIpvOzhF6sZz4B nnAUWSqGfYN9oERyUxM7s9TqSoe4AD2PkUabZmMSSsgjyMeddaXDiNGnffx3tLBYmEyPNggl6nR 1UsibNFx0TJwdR09OXYoW/n4= X-Received: by 127.0.0.2 with SMTP id Eu2mYY7687511xQNUEGOL6rd; Mon, 26 Feb 2024 22:39:07 -0800 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web11.6806.1709015946071909371 for ; Mon, 26 Feb 2024 22:39:06 -0800 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8CxWOhkgt1lltgRAA--.25988S3; Tue, 27 Feb 2024 14:34:12 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8DxdMxegt1lh11HAA--.9000S3; Tue, 27 Feb 2024 14:34:06 +0800 (CST) Message-ID: <7eed45e5-157a-4ef8-b71a-cd7460ac5a91@loongson.cn> Date: Tue, 27 Feb 2024 14:34:06 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v1] SctPkg: Fixed a pinter error in DevicePathFromTextBBTestCoverage.c From: "Chao Li" To: devel@edk2.groups.io, heinrich.schuchardt@canonical.com Cc: G Edhaya Chandran , Barton Gao , Carolyn Gjertsen Reply-To: devel@edk2.groups.io,lichao@loongson.cn References: <20240226101752.247067-1-lichao@...> <17B7934ED7B1BC99.684@groups.io> In-Reply-To: <17B7934ED7B1BC99.684@groups.io> X-CM-TRANSID: AQAAf8DxdMxegt1lh11HAA--.9000S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQAACGXcS+QJPAAAsq X-Coremail-Antispam: 1Uk129KBj93XoWxZFWUuF4DtFWktF18Jr45XFc_yoWrJFW8pF nYqF1jvFZFqr4IganI9a1xKrykXrs5X3W8Ga15Wa1jkrn3JrZ0vFW09r9Ygw1UGr4kZw4Y qryjqrZrCF90ywcCm3ZEXasCq-sJn29KB7ZKAUJUUUU7529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ34c02F40En4AKxVAvwIkv4cxYr27v73VFW2AGmfu7bjvjm3AaLaJ3UjIY CTnIWjp_UUUYJ7kC6x804xWl14x267AKxVWUJVW8JwAFc2x0x2IEx4CE42xK8VAvwI8IcI k0rVWrJVCq3wAFIxvE14AKwVWUXVWUAwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK 021l84ACjcxK6xIIjxv20xvE14v26r1I6r4UM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26r 1j6r4UM28EF7xvwVC2z280aVAFwI0_Gr1j6F4UJwA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_ Gr1j6F4UJwAaw2AFwI0_Jrv_JF1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqjxCEc2xF0c Ia020Ex4CE44I27wAqx4xG62kEwI0EY4vaYxAvb48xMcIj6xIIjxv20xvE14v26r126r1D McIj6I8E87Iv67AKxVW8JVWxJwAm72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41l74 80Y4vEI4kI2Ix0rVAqx4xJMxAIw28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4U MxCIbckI1I0E14v26r1Y6r17MI8I3I0E5I8CrVAFwI0_JrI_JrWlx2IqxVCjr7xvwVAFwI 0_JrI_JrWlx4CE17CEb7AF67AKxVWUAVWUtwCIc40Y0x0EwIxGrwCI42IY6xIIjxv20xvE 14v26r1j6r1xMIIF0xvE2Ix0cI8IcVCY1x0267AKxVWUJVW8JwCI42IY6xAIw20EY4v20x vaj40_Jr0_JF4lIxAIcVC2z280aVAFwI0_Jr0_Gr1lIxAIcVC2z280aVCY1x0267AKxVWU JVW8JbIYCTnIWIevJa73UjIFyTuYvjxU49YwUUUUU Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 0vv1nAbt19ESFDXSIrspE1kgx7686176AA= Content-Type: multipart/alternative; boundary="------------MrqPJxH4xF5yrFHKq3aC1QBs" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=JaxPxsPP; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --------------MrqPJxH4xF5yrFHKq3aC1QBs Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Heinrich, I guess it is not allowed to make them the same now, so I submitted V2, added your recomended fix message to the commit message, and also added more reviewers. Thanks, Chao On 2024/2/27 09:15, Chao Li wrote: > > Hi Heinrich, > > On 2024/2/26 19:02, Heinrich Schuchardt wrote: >> On 26.02.24 11:17, Chao Li wrote: >>> DevicePathFromTextBBTextCoverage.c function CreateDNSDeviceNode has a >>> bug, code: >>> >>> SctStrToIPv4Addr (&IpStr1, (EFI_IPv4_ADDRESS *)(DNS + sizeof >>> (DNS_DEVICE_PATH))); >>> >>> DNS is a pointer, which is increased by a structure size and converted >>> to EFI_IPv4_ADDRESS*, which will point to an unknown address. So fix >>> it. >> >> Hello Chao, >> >> thanks for diving into this. >> >> Please, add >> >> Fixes: 847e0363e846 ("SctPkg: Fix the UefiSct >> -Wincompatible-pointer-types warnings") >> >>> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4712 >>> >>> Cc: G Edhaya Chandran >>> Cc: Barton Gao >>> Cc: Carolyn Gjertsen >>> Signed-off-by: Chao Li >>> --- >>>   .../BlackBoxTest/DevicePathFromTextBBTestCoverage.c       | 8 >>> ++++---- >>>   1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git >>> a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathFromText/BlackBoxTest/DevicePathFromTextBBTestCoverage.c >>> b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathFromText/BlackBoxTest/DevicePathFromTextBBTestCoverage.c >>> >>> index c96ee246..bd11c25a 100644 >>> --- >>> a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathFromText/BlackBoxTest/DevicePathFromTextBBTestCoverage.c >>> +++ >>> b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathFromText/BlackBoxTest/DevicePathFromTextBBTestCoverage.c >>> @@ -1734,13 +1734,13 @@ CreateDNSDeviceNode ( >>>     } >>> >>>     if (DNS->IsIPv6 == 0) { >>> -    SctStrToIPv4Addr (&IpStr1, (EFI_IPv4_ADDRESS *)(DNS + sizeof >>> (DNS_DEVICE_PATH))); >>> -    SctStrToIPv4Addr (&IpStr2, (EFI_IPv4_ADDRESS *)(DNS + sizeof >>> (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS))); >>> +    SctStrToIPv4Addr (&IpStr1, (EFI_IPv4_ADDRESS *)((UINT8 *)DNS + >>> sizeof (DNS_DEVICE_PATH))); >>> +    SctStrToIPv4Addr (&IpStr2, (EFI_IPv4_ADDRESS *)((UINT8 *)DNS + >>> sizeof (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS))); >>>     } >>> >>>     if (DNS->IsIPv6 == 1) { >>> -    SctStrToIPv6Addr (&IpStr1, (EFI_IPv6_ADDRESS *)(DNS + sizeof >>> (DNS_DEVICE_PATH))); >>> -    SctStrToIPv6Addr (&IpStr2, (EFI_IPv6_ADDRESS *)(DNS + sizeof >>> (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS))); >>> +    SctStrToIPv6Addr (&IpStr1, (EFI_IPv6_ADDRESS *)((UINT8 *)DNS + >>> sizeof (DNS_DEVICE_PATH))); >>> +    SctStrToIPv6Addr (&IpStr2, (EFI_IPv6_ADDRESS *)((UINT8 *)DNS + >>> sizeof (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS))); >> >> The change looks correct. >> >> I would prefer using the fields of the DNS_DEVICE_PATH typedef from >> MdePkg/Include/Protocol/DevicePath.h to avoid the conversions: >> >> > +    SctStrToIPv4Addr (&IpStr1, &DNS->DnsServerIp[0].v4) >> > +    SctStrToIPv4Addr (&IpStr2, &DNS->DnsServerIp[1].v4) >> >> > +    SctStrToIPv6Addr (&IpStr1, &DNS->DnsServerIp[0].v6) >> > +    SctStrToIPv6Addr (&IpStr2, &DNS->DnsServerIp[1].v6) > I also prefer this way, but the definition of DNS in > UEFI/Protocol/DevicePath.h is not same as the MdePkg version, should > we make them the same? >> >> Best regards >> >> Heinrich >> >>>     } >>> >>>     return (EFI_DEVICE_PATH_PROTOCOL *) DNS; >>> -- >>> 2.27.0 >> >> >> >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116005): https://edk2.groups.io/g/devel/message/116005 Mute This Topic: https://groups.io/mt/104579419/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- --------------MrqPJxH4xF5yrFHKq3aC1QBs Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi Heinrich,

I guess it is not allowed to make them the same now, so I submitted V2, added your recomended fix message to the commit message, and also added more reviewers.


Thanks,
Chao
On 2024/2/27 09:15, Chao Li wrote:

Hi Heinrich,

On 2024/2/26 19:02, Heinrich Schuchardt wrote:
On 26.02.24 11:17, Chao Li wrote:
DevicePathFromTextBBTextCoverage.c function CreateDNSDeviceNode has a
bug, code:

SctStrToIPv4Addr (&IpStr1, (EFI_IPv4_ADDRESS *)(DNS + sizeof (DNS_DEVICE_PATH)));

DNS is a pointer, which is increased by a structure size and converted
to EFI_IPv4_ADDRESS*, which will point to an unknown address. So fix it.

Hello Chao,

thanks for diving into this.

Please, add

Fixes: 847e0363e846 ("SctPkg: Fix the UefiSct -Wincompatible-pointer-types warnings")


BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4712

Cc: G Edhaya Chandran <Edhaya.Chandran@...>
Cc: Barton Gao <gaojie@...>
Cc: Carolyn Gjertsen <Carolyn.Gjertsen@...>
Signed-off-by: Chao Li <lichao@...>
---
  .../BlackBoxTest/DevicePathFromTextBBTestCoverage.c       | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathFromText/BlackBoxTest/DevicePathFromTextBBTestCoverage.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathFromText/BlackBoxTest/DevicePathFromTextBBTestCoverage.c
index c96ee246..bd11c25a 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathFromText/BlackBoxTest/DevicePathFromTextBBTestCoverage.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathFromText/BlackBoxTest/DevicePathFromTextBBTestCoverage.c
@@ -1734,13 +1734,13 @@ CreateDNSDeviceNode (
    }

    if (DNS->IsIPv6 == 0) {
-    SctStrToIPv4Addr (&IpStr1, (EFI_IPv4_ADDRESS *)(DNS + sizeof (DNS_DEVICE_PATH)));
-    SctStrToIPv4Addr (&IpStr2, (EFI_IPv4_ADDRESS *)(DNS + sizeof (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS)));
+    SctStrToIPv4Addr (&IpStr1, (EFI_IPv4_ADDRESS *)((UINT8 *)DNS + sizeof (DNS_DEVICE_PATH)));
+    SctStrToIPv4Addr (&IpStr2, (EFI_IPv4_ADDRESS *)((UINT8 *)DNS + sizeof (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS)));
    }

    if (DNS->IsIPv6 == 1) {
-    SctStrToIPv6Addr (&IpStr1, (EFI_IPv6_ADDRESS *)(DNS + sizeof (DNS_DEVICE_PATH)));
-    SctStrToIPv6Addr (&IpStr2, (EFI_IPv6_ADDRESS *)(DNS + sizeof (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS)));
+    SctStrToIPv6Addr (&IpStr1, (EFI_IPv6_ADDRESS *)((UINT8 *)DNS + sizeof (DNS_DEVICE_PATH)));
+    SctStrToIPv6Addr (&IpStr2, (EFI_IPv6_ADDRESS *)((UINT8 *)DNS + sizeof (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS)));

The change looks correct.

I would prefer using the fields of the DNS_DEVICE_PATH typedef from MdePkg/Include/Protocol/DevicePath.h to avoid the conversions:

> +    SctStrToIPv4Addr (&IpStr1, &DNS->DnsServerIp[0].v4)
> +    SctStrToIPv4Addr (&IpStr2, &DNS->DnsServerIp[1].v4)

> +    SctStrToIPv6Addr (&IpStr1, &DNS->DnsServerIp[0].v6)
> +    SctStrToIPv6Addr (&IpStr2, &DNS->DnsServerIp[1].v6)
I also prefer this way, but the definition of DNS in UEFI/Protocol/DevicePath.h is not same as the MdePkg version, should we make them the same?

Best regards

Heinrich

    }

    return (EFI_DEVICE_PATH_PROTOCOL *) DNS;
--
2.27.0





_._,_._,_
Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#116005) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------MrqPJxH4xF5yrFHKq3aC1QBs--