From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web11.1935.1585729381632246025 for ; Wed, 01 Apr 2020 01:23:01 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eAZZZ7EZ; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585729380; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=doZ6PML0iHQmq8JQJ2G/KDgOXOZYLKpMJHgT7W8sX5g=; b=eAZZZ7EZKpGewHhPT1ZbE9OLUaZWZ6XB+sIKVWtrC7taZm7C7yTPKAmFKE5Ipta30v1Neb +gUFRmcZKpiRGSqta8LjkWZuUsyeuK6uz6FUyJjk+AI+52JIZ94Oux3Q6X9PloEKBU5GCz cGzOvt2KK4GlQ+AYuodVYMfDI99mUwQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-345-PQ-UNfYFO0yk3zwY67NlrA-1; Wed, 01 Apr 2020 04:22:53 -0400 X-MC-Unique: PQ-UNfYFO0yk3zwY67NlrA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3E68313FB; Wed, 1 Apr 2020 08:22:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-94.ams2.redhat.com [10.36.114.94]) by smtp.corp.redhat.com (Postfix) with ESMTP id 393E31001B07; Wed, 1 Apr 2020 08:22:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast To: Liran Alon , devel@edk2.groups.io, sean.brogan@microsoft.com References: <20200331110452.51992-1-liran.alon@oracle.com> <22866.1585670000047029434@groups.io> <4b90b41a-2cf1-8b6e-6619-1c652820265e@redhat.com> <5221e324-9f86-24b4-56a0-301a948151f8@oracle.com> From: "Laszlo Ersek" Message-ID: <7f6e5958-53f5-43fe-f452-2a5ee95050e0@redhat.com> Date: Wed, 1 Apr 2020 10:22:50 +0200 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: <5221e324-9f86-24b4-56a0-301a948151f8@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/01/20 00:13, Liran Alon wrote: >=20 > On 01/04/2020 0:56, Laszlo Ersek wrote: >> On 03/31/20 17:53, Sean via Groups.Io wrote: >>> A couple of thoughts. >>> 1. I would suggest that ASSERT should not be the only protection for >>> an invalid operation as ASSERT is usually disabled on release builds. >>> 2. We do have a library to make this more explicit and common. >>> https://urldefense.com/v3/__https://github.com/tianocore/edk2/blob/mast= er/MdePkg/Include/Library/SafeIntLib.h*L548__;Iw!!GqivPVa7Brio!LNp76poqa4dl= y7M8C8F3aX66wzZA68yStF_9jS-pD3izw3uJ24i_mDygFJEBsLc$ >>> >> In this case, when "Response->ScsiStatus" does not fit in >> "Packet->TargetStatus", the device model is obviously (and blatantly) >> misbehaving, so I would agree with Liran that trying to recover from >> that (or to cover it up with a nice error code passed out) is futile. > Exactly. >> >> I do agree with the observation however that ASSERT()s disappear from >> RELEASE builds. >> >> Mike Kinney taught me a pattern to deal with this. There are various >> ways to write it; one example (for this case) is: >> >> =C2=A0=C2=A0 ASSERT (Response->ScsiStatus <=3D MAX_UINT8); >> =C2=A0=C2=A0 if (Response->ScsiStatus > MAX_UINT8) { >> =C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop (); >> =C2=A0=C2=A0 } >> =C2=A0=C2=A0 Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; >> >> An alternative way to write it is (by moving the ASSERT into the block): >> >> =C2=A0=C2=A0 if (Response->ScsiStatus > MAX_UINT8) { >> =C2=A0=C2=A0=C2=A0=C2=A0 ASSERT (Response->ScsiStatus <=3D MAX_UINT8); >> =C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop (); >> =C2=A0=C2=A0 } >> =C2=A0=C2=A0 Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; >> >> Yet another (simply assert FALSE in the block): >> >> =C2=A0=C2=A0 if (Response->ScsiStatus > MAX_UINT8) { >> =C2=A0=C2=A0=C2=A0=C2=A0 ASSERT (FALSE); >> =C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop (); >> =C2=A0=C2=A0 } >> =C2=A0=C2=A0 Packet->TargetStatus =3D (UINT8)Response->ScsiStatus; >> >> >> Why: >> >> - in DEBUG builds, the assertion failure will be logged, and the proper >> assertion failure action will be triggered (CpuDeadLoop / exception / >> ..., as configured by the platform) >> >> - in RELEASE builds, we'll still hang, and might have a chance to >> investigate (get a stack dump perhaps). >> >> Regarding SafeIntLib, I'm a fan in general. In this case, I did not >> think of it (possible integer truncations seem so rare in this driver). >> For this patch, I'm OK either way (with or without using SafeIntLib), as >> long as we add both the ASSERT and the explicit CpuDeadLoop (either >> variant of the three). >> >> Thanks >> Laszlo > Honestly, I don't quite understand why using SafeIntLib is useful in > this case. > It just internally does even more branches and checks for exactly same > overflow and return RETURN_BUFFER_TOO_SMALL in case value is bigger than > MAX_UINT8. > Externally, I would still need to do a check on SafeUint16ToUint8() > return-value. So what's the benefit?... Seems to me to just be an > useless overhead. > I believe checking against MAX_UINT8 and casting immediately one line > afterwards, is explicit enough. >=20 > Regarding above comment that ASSERT() doesn't do anything for RELEASE > builds: > The point in ASSERT() is to be able to check a condition early to assist > debugging but not worth putting this condition in RELEASE as it should > never happen and just waste CPU cycles. > I thought this is the case we have here. If a weird ScsiStatus would > return, it is highly unlikely that boot would just succeed as usual, and > if it does, does the user really care? > In contrast, when boot fails because of this, it makes sense to build in > DEBUG in attempt to verify what happened. > Note that if this condition will ever evaluate to FALSE (i.e. ScsiStatus > is bigger than MAX_UINT8), then it is probably very deterministic. As it > means PVSCSI device emulation on host is broken > and broke because of a very specific commit on host userspace VMM (E.g. > QEMU). > Therefore, I still think an ASSERT() is what fits here best. But if you > still think otherwise, then I have no objection to change it (Or Laszlo > change it when applying). OK. Based on your answer, and also on Sean's , for this patch: Reviewed-by: Laszlo Ersek Thanks! Laszlo > I would say though, that if the pattern above is common, why isn't there > a macro in EDK2 for that instead of manually writing it? Something like: >=20 > #define RELEASE_ASSERT(Cond) \ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 if (!(Cond)) {=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 ASSERT (FALSE);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 \ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 CpuDeadLoop ();=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 \ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 } >=20 > That would be more elegant. >=20 > -Liran >=20