From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.11243.1583328799191407927 for ; Wed, 04 Mar 2020 05:33:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=a/SqB/ZZ; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583328798; 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=9g7+2A/M4r+7GuchyQ609M3bvW2Y2+7iN23gH2esr7Y=; b=a/SqB/ZZX6cOzS7Yw7Z5AD22YDv3Of6vM3PDYiLBFvdhLvZboBqzOqYOoR9+vi4b4S4cXb uRRfv//yLoYJWNk0LXw4is/5u2s6sMUN6Efje1NkBY7gBSNXBZt6qIcKHVoqFhuThBjfzB Aae5j1AK1x4Cajqdq7eCstrv0QcsYu8= 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-413-SoG2aaCUNXO1N9ikNTxH-Q-1; Wed, 04 Mar 2020 08:33:12 -0500 X-MC-Unique: SoG2aaCUNXO1N9ikNTxH-Q-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 18A6D800D48; Wed, 4 Mar 2020 13:33:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-59.ams2.redhat.com [10.36.117.59]) by smtp.corp.redhat.com (Postfix) with ESMTP id EDCBD73879; Wed, 4 Mar 2020 13:33:06 +0000 (UTC) Subject: Re: Disabling safe string constraint assertions To: Vitaly Cheptsov , devel@edk2.groups.io, =?UTF-8?Q?Marvin_H=c3=a4user?= , Mike Kinney , Andrew Fish Cc: "Gao, Liming" , "Gao, Zhichao" References: <17370CE3-AA53-4858-B497-1AB93DA709D3@ispras.ru> From: "Laszlo Ersek" Message-ID: <86d328d2-e929-379a-e6dd-69968a064f13@redhat.com> Date: Wed, 4 Mar 2020 14:33:06 +0100 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: <17370CE3-AA53-4858-B497-1AB93DA709D3@ispras.ru> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/03/20 22:12, Vitaly Cheptsov wrote: > Hello, >=20 > I am creating a new thread for this issue, as for some reason half of my = messages do not display in the web-interface and some of e-mail clients in = the previous one. It seems that somebody sent broken HTML code to groups.io= , and it did not like to be quoted. It will be quite ni= ce if all the messages sent are plain-text to avoid such issues in the futu= re. >=20 > Back to the issue, we want to make constraint assertions (SAFE_STRING_CON= STRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as t= hey break our ability to use some of BaseLib interfaces to verify untrusted= user data in UEFI Applications. The background of this problem is covered = in a bugzilla[1], and was also extensively discussed in the last proposed p= atch discussion thread[2]. We want the solution to the problem to land in t= he next stable tag: edk2-stable202005, and can submit the patch. >=20 > Currently there is no clear decision on what approach to take, as there a= re three different proposals to implement: >=20 > 1) Andrew Fish=E2=80=99s approach with C11-like constraint handler[3]. > 2) Marvin H=C3=A4user=E2=80=99s approach with return codes and constraint= assertions being moved to the caller side[4]. > 3) My approach suggesting DebugLib interface simplification, permitting u= s to resolve the problem and keep backwards compatibility[5]. >=20 > My personal opinion is that: >=20 > =E2=80=94 Marvin=E2=80=99s way is very interesting, but it would require = implementing code on the caller side basically for every call, and this wou= ld requires extra programming effort. It may also be uneasy to teach people= how to do this right, and this does not strictly provide a good solution f= or situations, where nesting is more than 2 (i.e. when user code calls libr= ary code, which itself calls library code). Because of these downsides I am= afraid it is impractical to adopt it in EDK II. > =E2=80=94 Andrew=E2=80=99s way is reasonable from the C standard point of= view, but it does not work quite right with reentrancy and I am not positi= ve we need the dynamic handling, especially because of DEBUG/RELEASE build = confusion. I explained this in my previous e-mail, which I attached to the = end of this message as quote, since groups.io ate the p= revious send. Other than that, I would say that I can implement this approa= ch if there is no other option. > =E2=80=94 My own approach makes sense, as it reduces the amount of code i= n every DebugLib and actually simplifies the development in the future. I b= elieve currently this is the most reasonable route we can take, and suggest= other parties to consider it. >=20 > Since the three of us each have their own suggestions, I ask Mike and oth= ers to rejoin our discussions and give their opinions quickly, so that we c= an choose, perhaps vote, and proceed with the implementation. Thanks for yo= ur dedication. I support the original proposal given in: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054#c0 also known as the v3 posting: * [edk2-devel] [PATCH v3 1/1] MdePkg: Add PCD to disable safe string constraint assertions https://edk2.groups.io/g/devel/message/52838 http://mid.mail-archive.com/20200103171242.63839-2-vit9696@protonmail.com With the following tweak: I think we should rather introduce a new bit in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD. (Note that both PcdDebugPropertyMask and the propsed new PCD support precisely the following PCD access methods: PcdsFixedAtBuild, PcdsPatchableInModule.) I don't feel too strongly about this part (i.e., whether we introduce a new PCD, or a new bit in PcdDebugPropertyMask), as long as we make sure that the access method can *never* become dynamic. I do prefer quite strongly the original proposal, at a higher level. Here's why I support the original proposal: - In a brand new code base (or brand new set of APIs), I would fight tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up to the caller to check return values. Also, additional wrapper functions/macros can be offered *on top* of the base APIs that internalize the ASSERT()s for special use cases. But this ship has sailed, for edk2. - I'm unfriendly towards callbacks. They make the behavior of code implicit rather than explicit, and implicit is more difficult to reason about. IMO callbacks should be considered a last resort only. Just my two cents, because I've been asked to comment. Thanks Laszlo