From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.45]) by mx.groups.io with SMTP id smtpd.web10.10223.1589302993617069010 for ; Tue, 12 May 2020 10:03:14 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ispras.ru, ip: 83.149.199.45, mailfrom: cheptsov@ispras.ru) Received: from [127.0.0.1] (unknown [77.232.9.83]) by mail.ispras.ru (Postfix) with ESMTPSA id 5AE71CD466; Tue, 12 May 2020 20:03:12 +0300 (MSK) From: "Vitaly Cheptsov" Message-Id: <33B9E11E-5402-4F21-9210-853FC163AD82@ispras.ru> Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) Subject: Re: [PATCH V4 00/27] Disabling safe string constraint assertions Date: Tue, 12 May 2020 20:03:11 +0300 In-Reply-To: <44ac1ca1-953a-21a2-0c9e-c83aca153b0b@redhat.com> Cc: "devel@edk2.groups.io" , Andrew Fish , =?utf-8?Q?Marvin_H=C3=A4user?= , "Gao, Liming" , "Gao, Zhichao" To: "Kinney, Michael D" , Laszlo Ersek References: <20200511154121.3878-1-cheptsov@ispras.ru> <44ac1ca1-953a-21a2-0c9e-c83aca153b0b@redhat.com> X-Mailer: Apple Mail (2.3608.80.23.2.2) X-Groupsio-MsgNum: 59336 Content-Type: multipart/signed; boundary="Apple-Mail=_8BD65F09-C3A7-467D-9202-F73E37B2C707"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_8BD65F09-C3A7-467D-9202-F73E37B2C707 Content-Type: multipart/alternative; boundary="Apple-Mail=_CC5F070F-007D-4310-936E-994EAF10C596" --Apple-Mail=_CC5F070F-007D-4310-936E-994EAF10C596 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hello everyone, Let me try in order. After an internal review I added the following to SafeString commit = message: Note, for packages with constraint assertions disabled this change turn off the assertions on constraint violations. This is obvious, and I believe everyone agreed it is not healthy except = for some private packages but I guess it is worth mentioning, so that it = is cared in these private packages as necessary. The latest version of the changes is on GitHub branch = (https://github.com/vit9696/edk2/tree/constraint = ). I also resubmitted = the V5 of the patchset including all the requested changes. > You have included the from > MdePkg/Include/Library/DebugLib.h. It is very rare for a > lib class to include another lib class. This means that a module > that has a dependency on the DebugLib class inherits a hidden > dependency on the DebugCommonLib class. For module INF files, > we require the INF file to list all the lib classes that the > module sources directly use. Since a module that uses the > DebugLib uses the ASSERT() and DEBUG() macros, all the APIs > that the ASSERT() and DEBUG() macros use are also directly > used by the module. With this patch series, these macros > now use the DebugCommonLib class APIs, which means any module > that uses the DebugLib also directly uses the DebugCommonLib. > The INF files for all modules that use the DebugLib should > also be updated to list the DebugCommonLib. If we go down > that path, then it would be cleaner for the modules to include > both DebugLib.h and DebugCommonLib.h so the list of includes > matches the list of lib classes in the INF file. This would > be an even much larger change than the patch series already > under review. I do not think it is unusual to include one library header from the = other. That=E2=80=99s what we regularly do in our code. I also do not = agree that we need to add a dependency on a library consumed by the = other library. This is an indirect dependency. Just like Laszlo says. > In order to address the original problem statement and > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054 = > Perhaps we should go back to the original proposal that > adds one new PCD so the string APIs in the BaseLib can > filter out ASSERT() conditions for UEFI Applications that > want return status behavior without ASSERT() behavior. If you believe it is not possible to merge the current patch into May = release, I am fine to submit a small change that will add a PCD to = protect SafeString. However, I am not very happy about it as it may take = a considerable amount of time to get rid of this PCD in the future, and = we (and likely other people) will have to change the code multiple = times. I really want to resolve this sick situation with both BaseLib = and DebugLib once and forever, as the need to change the hell of = DebugLibs all over the place for a small thing is very very wrong. > Note: unfortunately, the edk2-platforms tree contains a huge number of > DebugLib class resolutions (124 resolutions in 48 files, at commit > bfa32ac70a75), and I think the final patch in the present series will > break those platforms unless you post patches for them too. >=20 > IMO this is another good example why edk2-platforms should *not* exist > as a separate repository. I agree that separating edk2-platforms is very sick as well. I can send = the patches on them too, but due to their amount (just like you said it = will be almost half a hundred) I would rather make a branch and then = just merge it. Same for EDK II (e.g. somewhere in edk2-staging). In = addition to that is very hard for me to read the review of the patches = by e-mails, and spamming the mailing list feels also not so great. > (... Please consider adding Cc: tags to the commit messages, so that > each patch email reaches the subject package owners directly too. = Anyway > that's for the future.) Fixed. > I think the very last hunk in this patch belongs in patch#26 ("MdePkg: > Introduce assertion on constraint debug mask bit"), which is where = BIT6 > is actually defined for PcdDebugPropertyMask. > (2) The "MdePkg/MdePkg.uni" file update from patch#1 belongs here, = IMO. Fixed. > I think the VALID_ARCHITECTURES comment should be dropped altogether. >=20 > At least the current arch list "IA32 X64 EBC" is incorrect; first > because ARM and AARCH64 are missing (despite the series -- correctly = -- > patching Arm*Pkg and such), and second because -- I *think* -- EBC is = no > longer an arch natively supported by edk2. (Mike and Liming will = correct > me on that, if needed.) Fixed. > (1) The other two INF files in the same directory should get the same > update. (I.e., a dependency on DebugCommonLib.) Nice catch, fixed. > (2) I believe it should be possible to remove > "PcdFixedDebugPrintErrorLevel" from all three INF files in the same > directory. (The only reference, which is being removed, is in > DebugPrintLevelEnabled().) This issue is common across the patches, fixed everywhere. > (1) Ugh, sorry, my point (1) was going to be about out two instances = of > the same typo: >=20 > s/constrain/constraint/ There also were a couple of cases with constrait. Fixed everything. Best regards, Vitaly > 12 =D0=BC=D0=B0=D1=8F 2020 =D0=B3., =D0=B2 12:50, Laszlo Ersek = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > On 05/12/20 00:40, Kinney, Michael D wrote: >> Vitaly, >>=20 >> Thank you for the contribution. >>=20 >> There are a couple points about this approach that need to be = discussed. >>=20 >> You have included the from >> MdePkg/Include/Library/DebugLib.h. >=20 > Right, I've noticed it. I agree it's unusual. I didn't think it was = wrong. >=20 >> It is very rare for a >> lib class to include another lib class. This means that a module >> that has a dependency on the DebugLib class inherits a hidden >> dependency on the DebugCommonLib class. >=20 > I agree. >=20 > I think it should be fine. Any header H1 should include such further > headers H2, H3, ... Hn that are required for making the interfaces > declared in H1 usable in client modules. >=20 >> For module INF files, >> we require the INF file to list all the lib classes that the >> module sources directly use. >=20 > Yes, keyword being "directly". >=20 >> Since a module that uses the >> DebugLib uses the ASSERT() and DEBUG() macros, all the APIs >> that the ASSERT() and DEBUG() macros use are also directly >> used by the module. >=20 > I believe this is where I disagree. The replacement texts of the > ASSERT() and DEBUG() function-like macros are internals of the > DebugLib.h lib class header, in my opinion. Those internals place > requirements on specific DebugLib instances, not on DebugLib class > consumers. >=20 > In other words, when writing a new DebugLib instance, the implementor > has to ensure that the ASSERT() and DEBUG() macros, as defined in the > DebugLib class header, will continue working in DebugLib consumer > modules. The implementor may then choose to make the new DebugLib > instance dependent on the (singleton) DebugCommonLib instance, for > example. (This is being done in patches #3, #4, #16, maybe more.) The > DebugLib consumer module will inherit that dependency, and everything > will work. >=20 > Just because ASSERT() and DEBUG() are function-like macros and not > actual functions, I don't think the INF file requirements in > DebugLib-consumer modules should change. >=20 >> With this patch series, these macros >> now use the DebugCommonLib class APIs, which means any module >> that uses the DebugLib also directly uses the DebugCommonLib. >=20 > In my opinion: indirectly. >=20 > Thanks, > Laszlo >=20 --Apple-Mail=_CC5F070F-007D-4310-936E-994EAF10C596 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hello= everyone,

Let me = try in order.

After an internal review I added the following to SafeString = commit message:
Note, for packages with constraint = assertions disabled this change
turn = off the assertions on constraint violations.
This is obvious, and I believe everyone agreed it is not = healthy except for some private packages but I guess it is worth = mentioning, so that it is cared in these private packages as = necessary.

The = latest version of the changes  is on GitHub branch = (https://github.com/vit9696/edk2/tree/constraint). = I also resubmitted the V5 of the patchset including all the requested = changes.


You have included the <Library/DebugCommonLib.h> = from 
MdePkg/Include/Library/DebugLib.h.  It is = very rare for a 
lib class to include another lib = class.  This means that a module
that has a dependency on = the DebugLib class inherits a hidden
dependency on the = DebugCommonLib class.  For module INF files,
we = require the INF file to list all the lib classes that the
module sources directly use.  Since a module that uses = the
DebugLib uses the ASSERT() and DEBUG() macros, all the = APIs
that the ASSERT() and DEBUG() macros use are also = directly
used by the module.  With this patch series, = these macros
now use the DebugCommonLib class APIs, which = means any module
that uses the DebugLib also directly = uses the DebugCommonLib.
The INF files for all modules that = use the DebugLib should
also be updated to list the = DebugCommonLib.  If we go down
that path, then it would = be cleaner for the modules to include
both DebugLib.h and = DebugCommonLib.h so the list of includes
matches the list of lib = classes in the INF file.  This would
be an even much larger = change than the patch series already
under = review.

I do not = think it is unusual to include one library header from the other. = That=E2=80=99s what we regularly do in our code. I also do not agree = that we need to add a dependency on a library consumed by the other = library. This is an indirect dependency. Just like Laszlo = says.

In = order to address the original problem statement and 
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054
Perhaps we should go back to the original proposal = that 
adds one new PCD so the string APIs in the = BaseLib can
filter out ASSERT() conditions for UEFI = Applications that
want return status behavior without = ASSERT() behavior.

If you believe it is not possible to = merge the current patch into May release, I am fine to submit a small = change that will add a PCD to protect SafeString. However, I am not very = happy about it as it may take a considerable amount of time to get rid = of this PCD in the future, and we (and likely other people) will have to = change the code multiple times. I really want to resolve this sick = situation with both BaseLib and DebugLib once and forever, as the need = to change the hell of DebugLibs all over the place for a small thing is = very very wrong.

Note:= unfortunately, the edk2-platforms tree contains a huge number = of
DebugLib class resolutions (124 resolutions in 48 files, = at commit
bfa32ac70a75), and I think the final patch in the = present series will
break those platforms unless you post = patches for them too.

IMO this is another good example = why edk2-platforms should *not* exist
as a separate = repository.

I = agree that separating edk2-platforms is very sick as well. I can send = the patches on them too, but due to their amount (just like you said it = will be almost half a hundred) I would rather make a branch and then = just merge it. Same for EDK II (e.g. somewhere in edk2-staging). In = addition to that is very hard for me to read the review of the = patches by e-mails, and spamming the mailing list feels also not so = great.

(... = Please consider adding Cc: tags to the commit messages, so = that
each patch email reaches the subject package owners = directly too. Anyway
that's for the = future.)

Fixed.

I think the very last hunk in this patch belongs in = patch#26 ("MdePkg:
Introduce assertion on constraint debug = mask bit"), which is where BIT6
is actually defined for = PcdDebugPropertyMask.

(2) = The "MdePkg/MdePkg.uni" file update from patch#1 belongs here, = IMO.

Fixed.

I = think the VALID_ARCHITECTURES comment should be dropped = altogether.

At least the current arch list "IA32 X64 EBC" = is incorrect; first
because ARM and AARCH64 are missing = (despite the series -- correctly --
patching Arm*Pkg and = such), and second because -- I *think* -- EBC is no
longer an arch natively supported by edk2. (Mike and Liming = will correct
me on that, if needed.)

Fixed.

(1) The other two INF files in the same directory should = get the same
update. (I.e., a dependency on = DebugCommonLib.)

Nice catch, fixed.

(2) I believe it should be possible to remove
"PcdFixedDebugPrintErrorLevel" from all three INF files in = the same
directory. (The only reference, which is being = removed, is in
DebugPrintLevelEnabled().)

This = issue is common across the patches, fixed everywhere.

(1) = Ugh, sorry, my point (1) was going to be about out two instances = of
the same typo:

s/constrain/constraint/

There also were a couple of cases with constrait. = Fixed everything.

Best = regards,
Vitaly

12 =D0=BC=D0=B0=D1=8F 2020 =D0=B3= ., =D0=B2 12:50, Laszlo Ersek <lersek@redhat.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0= =BB(=D0=B0):

On 05/12/20 00:40, Kinney, Michael D = wrote:
Vitaly,

Thank you for the contribution.

There are a couple points about this approach = that need to be discussed.

You have = included the <Library/DebugCommonLib.h> from
MdePkg/Include/Library/DebugLib.h.

Right, I've noticed it. I agree = it's unusual. I didn't think it was wrong.

It is very rare for a =
lib class to include another lib class.  This means = that a module
that has a dependency on the DebugLib class = inherits a hidden
dependency on the DebugCommonLib = class.

I agree.

I think it should be fine. Any header H1 = should include such further
headers H2, H3, ... Hn that = are required for making the interfaces
declared in H1 = usable in client modules.

For module INF files,
we require = the INF file to list all the lib classes that the
module = sources directly use.

Yes, = keyword being "directly".

Since a module that uses the
DebugLib uses the ASSERT() and DEBUG() macros, all the = APIs
that the ASSERT() and DEBUG() macros use are also = directly
used by the module.

I believe this is where I disagree. The replacement texts of = the
ASSERT() and DEBUG() function-like macros are = internals of the
DebugLib.h lib class header, in my = opinion. Those internals place
requirements on specific = DebugLib instances, not on DebugLib class
consumers.

In other words, when writing a new DebugLib = instance, the implementor
has to ensure that the ASSERT() = and DEBUG() macros, as defined in the
DebugLib class = header, will continue working in DebugLib consumer
modules. = The implementor may then choose to make the new DebugLib
instance dependent on the (singleton) DebugCommonLib = instance, for
example. (This is being done in patches #3, = #4, #16, maybe more.) The
DebugLib consumer module will = inherit that dependency, and everything
will work.

Just because ASSERT() and DEBUG() are = function-like macros and not
actual functions, I don't = think the INF file requirements in
DebugLib-consumer = modules should change.

With this patch series, these macros
now use the DebugCommonLib class APIs, which means any = module
that uses the DebugLib also directly uses the = DebugCommonLib.

In my opinion: = indirectly.

Thanks,
Laszlo


= --Apple-Mail=_CC5F070F-007D-4310-936E-994EAF10C596-- --Apple-Mail=_8BD65F09-C3A7-467D-9202-F73E37B2C707 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAl661s8ACgkQL8K2O86E yz4oZA/9F8X06pI20XitxDGQaPwEdfg+QUuMgtfc0FzKg4jX+hSgOn6g/ID3On+H 3xHMqdtaX7Scag2EO20tI6yZsS+rNYH3xyv+XNKerQ3AstKOoahoSQlh6bkM7vUc Mk7Srsqd6JWPZgq0xZ/5g/Ubef0ZdCm1R0hznNDKTiQuOj6p8Zo/dV2cqUkOTPaI 1/Ap68jom/qB1Hnf/ArSlvIL0itQgj7QOc9n62dTbB2qHU36scyaazpg/2pODiVE GL47AcMNJEck+lXgo/XZhnEmp7DAADeyRa/z3feMIQLVH8ZCJODaoXz8sFnFdehl bbQQ69rT6ghAvcokmOS41tK95+9W5GfouH2BKr+K1J3YQfRLaag1ZWGgzaYXJF/H AthbTcJgkPNjGgtwc4pHHMXmFLAB9YG38DdaG+AgnJr2jLleKHVFDrc7T+CUvjMu nakVHsR9g0/IScXKVSEJvzj+ZAMOgKRGb89sdnbCQz6QwzjNaxt5rBAE/gJfx2lN ZKqkow5sck1deRejF3KHSBQJQB72VnePjPaucnwNuMdfmqFb/N+g8lHqFwX33d1S dPd0ZDDT4GP9ByYQLIX7TSmW/2bFZ2rNBsgoVkXSnn+j/hglyt24GGbWM6P2qPSa 0vbV0gNL2gQRRqRtGY9NEYoOint04CphKlCwUOxLz4XQJeVWHUs= =yOeO -----END PGP SIGNATURE----- --Apple-Mail=_8BD65F09-C3A7-467D-9202-F73E37B2C707--