Re: [edk2-devel] [PATCH] OvmfPkg/PvScsiDxe: Fix VS2019 build error because of implicit cast

["Laszlo Ersek" <lersek@redhat.com>]

On 04/01/20 00:13, Liran Alon wrote:
> 
> 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/master/MdePkg/Include/Library/SafeIntLib.h*L548__;Iw!!GqivPVa7Brio!LNp76poqa4dly7M8C8F3aX66wzZA68yStF_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:
>>
>>    ASSERT (Response->ScsiStatus <= MAX_UINT8);
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>>
>> An alternative way to write it is (by moving the ASSERT into the block):
>>
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      ASSERT (Response->ScsiStatus <= MAX_UINT8);
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (UINT8)Response->ScsiStatus;
>>
>> Yet another (simply assert FALSE in the block):
>>
>>    if (Response->ScsiStatus > MAX_UINT8) {
>>      ASSERT (FALSE);
>>      CpuDeadLoop ();
>>    }
>>    Packet->TargetStatus = (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.
> 
> 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
<https://bugzilla.tianocore.org/show_bug.cgi?id=2651#c3>, for this patch:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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:
> 
> #define RELEASE_ASSERT(Cond) \
>                if (!(Cond)) {                \
>                  ASSERT (FALSE);        \
>                  CpuDeadLoop ();       \
>                }
> 
> That would be more elegant.
> 
> -Liran
>