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

["Liran Alon" <liran.alon@oracle.com>]

On 01/04/2020 1: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).
> 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
>
I would also mention that there are some bizzare code in EDK2 that 
defines it's own ASSERT() macro that just does CpuDeadLoop().
E.g. ArmVirtPkg/Library/ArmVirtDxeHobLib/HobLib.c and 
OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c)

-Liran