From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.12353.1589463318740199786 for ; Thu, 14 May 2020 06:35:18 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=c3TwswAy; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589463317; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=83nXUurdTzelxmWIqT30NhJGJt3o8QrkbAmlUltM1RY=; b=c3TwswAyFKwc5ptvKg+jr8w3bQ6HZqBqo1dnLUbm8nhajZCQqVFJu0B1ZLMflNRr2d89W8 JWGAwTTng5//OSHb9NwL51xKvdhNjm8DH/g8ZjS/bBjyzaoV8FpQqSeiPeKj8fqSpWWmy5 JOpY5szp/zUELIPNkg1QOerllWgIF2A= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-514-OwjdRlHDMZWLiU1yyght3Q-1; Thu, 14 May 2020 09:35:08 -0400 X-MC-Unique: OwjdRlHDMZWLiU1yyght3Q-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7BBD080058A; Thu, 14 May 2020 13:35:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-179.ams2.redhat.com [10.36.113.179]) by smtp.corp.redhat.com (Postfix) with ESMTP id A214460F8D; Thu, 14 May 2020 13:35:03 +0000 (UTC) Subject: Re: [PATCH V6 1/1] MdePkg: Fix SafeString performing assertions on runtime checks To: Vitaly Cheptsov , devel@edk2.groups.io Cc: Andrew Fish , Ard Biesheuvel , Bret Barkelew , "Brian J . Johnson" , Chasel Chiu , Jordan Justen , Leif Lindholm , Liming Gao , =?UTF-8?Q?Marvin_H=c3=a4user?= , Mike Kinney , Vincent Zimmer , Zhichao Gao References: <20200514092537.29609-1-cheptsov@ispras.ru> <20200514092537.29609-2-cheptsov@ispras.ru> From: "Laszlo Ersek" Message-ID: <48038e02-5340-b13c-a48b-7418b7cc2791@redhat.com> Date: Thu, 14 May 2020 15:35:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200514092537.29609-2-cheptsov@ispras.ru> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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 > CC: Ard Biesheuvel > CC: Bret Barkelew > CC: Brian J. Johnson > CC: Chasel Chiu > CC: Jordan Justen > CC: Laszlo Ersek > CC: Leif Lindholm > CC: Liming Gao > CC: Marvin Häuser > CC: Mike Kinney > CC: Vincent Zimmer > CC: Zhichao Gao > Signed-off-by: Vitaly Cheptsov > --- > 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