On 05/14/20 11:25, Vitaly Cheptsov wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054 > > Runtime checks returned via status return code should not work as > assertions to permit parsing not trusted data with SafeString > interfaces. > > CC: Andrew Fish <af...@apple.com> > CC: Ard Biesheuvel <ard.biesheu...@linaro.org> > CC: Bret Barkelew <bret.barke...@microsoft.com> > CC: Brian J. Johnson <brian.john...@hpe.com> > CC: Chasel Chiu <chasel.c...@intel.com> > CC: Jordan Justen <jordan.l.jus...@intel.com> > CC: Laszlo Ersek <ler...@redhat.com> > CC: Leif Lindholm <l...@nuviainc.com> > CC: Liming Gao <liming....@intel.com> > CC: Marvin Häuser <mhaeu...@outlook.de> > CC: Mike Kinney <michael.d.kin...@intel.com> > CC: Vincent Zimmer <vincent.zim...@intel.com> > CC: Zhichao Gao <zhichao....@intel.com> > Signed-off-by: Vitaly Cheptsov <vit9...@protonmail.com> > --- > MdePkg/Include/Library/BaseLib.h | 120 ++------------------ > MdePkg/Library/BaseLib/SafeString.c | 80 ------------- > 2 files changed, 7 insertions(+), 193 deletions(-)
This patch is a lot harder to review than it initially seems, for two reasons: - The existent comment blocks on the functions are absolutely huge. - The comment blocks are duplicated between the lib class header and the implementation. Therefore, I reviewed this patch as follows: displayed the lib class header diff to the left with 40 lines of context, displayed the implementation diff to the right (with 40 lines of context again), and compared those. I also made an effor to check whether the code behavior would be in sync with the updated comments. I'm not going to quote any context from the patch, because the context is too small for helping me convey my points; I'll just list my remarks. (1) The comment update on AsciiStrCpyS() only covers the header, the implementation's comment was missed. (2) Ditto for StrToIpv6Address(). (3) Ditto for StrToIpv4Address(). (4) Even in the lib class header, the leading comment on StrToIpv4Address() is not updated correctly. The third ASSERT() paragraph that's being removed (referencing PcdMaximumUnicodeStringLength) is not fully removed; only its last line is removed. (5) The comment update on StrToGuid() only covers the header, the implementation's comment was missed. (6) Ditto for StrHexToBytes(). (7) Regarding the UnicodeStrnToAsciiStrS() function, the comment updates between lib class header and implementation are inconsistent. Namely, the header file also received such comment updates that: (7aa) are semantic fixes (not related to whether we assert a particular condition or not), such as "MIN", and near "DestinationLength", (7b) are indentation / wrapping updates (near DestMax"). While these comment updates may be valid too, they should be split to separate patches. (8) The comment update on AsciiStrToUnicodeStrS() only covers the header, the implementation's comment was missed. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#59559): https://edk2.groups.io/g/devel/message/59559 Mute This Topic: https://groups.io/mt/74201257/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
