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 <[email protected]> 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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56818): https://edk2.groups.io/g/devel/message/56818 Mute This Topic: https://groups.io/mt/72673992/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
