Hi Marvin,
On 07/15/19 20:44, Marvin Häuser wrote:
> I feel like my rushed message mentioning 'MAX_ADDRESS' was misleading
> a little - the point with that was a potential index overflow (I may
> actually have meant 'MAX_UINTN', I am not sure about the details
> anymore) in the original code, I personally see a lot of sense in
> *not* checking whether the buffer wraps around (similarly the overlap
> condition). For one consistency with similar code, where no such
> checks exist, and then a sanity-trust in the caller (which, as this is
> a library function, is interior as opposed to an external protocol
> caller which should naturally be trusted less).
>
> Generally, I am rather confused about the edk2 trust model for static
> calls. A bunch of libraries verify input parameter sanity via ASSERTs,
> another bunch by runtime checks and appropriate return statuses. Is
> there any kind of policy I am unaware of?
Good points, and you are right, the landscape is not consistent.
I think the edk2 practice can be characterized as follows (again, the
picture is not uniform / consistent across the codebase):
(1) Functions are supposed to have detailed interface contracts, but
many don't. In some cases, cutting corners appears at least
moderately defensible, because a function can be really simple, and
writing documentation could be the larger part of the effort. I
still prefer if we document all new functions painstakingly.
(2) Caller responsibilities are frequently checked by callees. The
checks vary between ASSERT()s and explicit conditions / return
values. The 2nd kind is better, of course, and that is actually
exemplified by the UEFI spec.
Namely, a large proportion of EFI_INVALID_PARAMETER return codes
correspond to cases when the callee catches the caller breaking the
interface contract. See for example EFI_BOOT_SERVICES.CreateEvent().
In some other cases, similar efforts / spec requirements look
dubious (see EFI_BOOT_SERVICES.FreePool() -- "Buffer was invalid").
If the callee gets a pointer to freed storage, it can't even
*evaluate* that pointer without depending on undefined behavior. So,
in theory, there's no way to enforce the contract, beyond trusting
the caller, and so there can be no *substitute* for a detailed
contract; see (1).
I think the approach ("watch your caller") is generally acceptable
still, if there is not a large runtime cost, because the environment
is extremely unforgiving, and because the firmware can, in practice,
recover from *some* undefined behavior this way, even.
(3) Genuine failure conditions are occasionally checked with
ASSERT(). This should never be done.
Now, considering "wrapped arrays" and such -- obviously such a thing is
not even an object in C (no valid memory allocation would ever produce
it), so normally I would consider checks against it pointless.
However, in the present case, there are three arguments for including
the MAX_ADDRESS checks:
- The original code included similar MAX_ADDRESS checks, and I didn't
want to "weaken" the implementation in any way. (I simply didn't want
to defend such choices -- so I didn't make them.)
- The conditions are not costly, and as long as the buffer pointers are
into valid storage, the conditions do catch -- without invoking
undefined behavior -- *sizes* that would cause wrap-around. This is in
line with (2).
- This is MdePkg/BaseLib, so general distrust (with negligible runtime
cost) cannot hurt. Widely used library -- yes, not a protocol, but
still linked (statically) into a bunch of 3rd party code --, and
(again) an unforgiving environment.
I introduced the overlap check myself (IIRC). That was because:
- There is no language-level reason why decoding back into the same
buffer couldn't work -- it's just that this implementation doesn't aim
to support that. Hence the corresponding natural language restriction
in the interface contract. (In edk2, we don't have the "restrict"
keyword, from C99.) Otherwise a programmer might cleverly say, "aha,
base64 decoding never *inflates* data, so I'll just decode "in-place".
- And then, although it's a caller responsibility, catch the overlap
explicitly with RETURN_INVALID_PARAMETER, in line with the above: the
checks are cheap, and BaseLib is quite central.
Now I'm not trying to present any of this as "policy" -- just my 2
cents. I hope it makes sense.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43754): https://edk2.groups.io/g/devel/message/43754
Mute This Topic: https://groups.io/mt/32284614/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
