Either approach works for me.
I understand the desire to avoid code bloat that comes with the library.
The most common classes of issues I see at the moment are asserts being
misused for error handling (which is significant), general issues with
integer conversion/evaluation, and unsafe arithmetic operations.
I suppose this is in the spirit of Gerd's comment as I thought
additional library usage might increase awareness to help with the latter.
Do the MdeModulePkg / SMBIOS maintainers have a preference?
Thanks,
Michael
On 2/14/2023 8:01 AM, Gerd Hoffmann wrote:
On Mon, Feb 13, 2023 at 04:15:30PM +0000, Michael Brown wrote:
On 13/02/2023 15:48, Michael Kubacki wrote:
@@ -1608,9 +1610,12 @@ ParseAndAddExistingSmbiosTable (
//
// Make sure not to access memory beyond SmbiosEnd
//
- if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||
- (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
- {
+ Status = SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE),
&SafeIntResult);
+ if (EFI_ERROR (Status)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if ((SafeIntResult > (UINTN)SmbiosEnd.Raw) || (SafeIntResult <
(UINTN)Smbios.Raw)) {
return EFI_INVALID_PARAMETER;
}
Could this not be expressed much more cleanly as just:
if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < sizeof (SMBIOS_STRUCTURE)) {
return EFI_INVALID_PARAMETER;
}
without the need to hide a basic arithmetic operation behind an ugly wrapper
function and drag in an external library?
Well, the advantage of using SafeIntLib is that (a) it is obvious even
to untrained code readers what is going on here, and (b) it is hard to
get it wrong.
When looking at the quality some of the patches being posted to the list
have I'd say that is important to consider even if you personally have
no problems avoiding integer overflows without the help of SafeIntLib.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100178): https://edk2.groups.io/g/devel/message/100178
Mute This Topic: https://groups.io/mt/96938294/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
