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

[Laszlo Ersek]

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://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/SafeIntLib.h#L548

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.

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56780): https://edk2.groups.io/g/devel/message/56780
Mute This Topic: https://groups.io/mt/72673992/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-