Re: [PATCH V6 1/1] MdePkg: Fix SafeString performing assertions on runtime checks

["Laszlo Ersek" <lersek@redhat.com>]

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 <afish@apple.com>
> CC: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> CC: Bret Barkelew <bret.barkelew@microsoft.com>
> CC: Brian J. Johnson <brian.johnson@hpe.com>
> CC: Chasel Chiu <chasel.chiu@intel.com>
> CC: Jordan Justen <jordan.l.justen@intel.com>
> CC: Laszlo Ersek <lersek@redhat.com>
> CC: Leif Lindholm <leif@nuviainc.com>
> CC: Liming Gao <liming.gao@intel.com>
> CC: Marvin Häuser <mhaeuser@outlook.de>
> CC: Mike Kinney <michael.d.kinney@intel.com>
> CC: Vincent Zimmer <vincent.zimmer@intel.com>
> CC: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Vitaly Cheptsov <vit9696@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