[edk2-devel] [Patch v9 0/2] Disable safe string constraint assertions

Michael D Kinney posted 2 patches 4 days ago
Failed in applying to current master (apply log)
MdePkg/Include/Library/BaseLib.h              | 111 -----------------
MdePkg/Library/BaseLib/SafeString.c           | 115 +-----------------
.../UnitTest/Library/BaseLib/Base64UnitTest.c | 107 ++++++++++++++++
3 files changed, 110 insertions(+), 223 deletions(-)

[edk2-devel] [Patch v9 0/2] Disable safe string constraint assertions

Posted by Michael D Kinney 4 days ago
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054

V9 Update unit test comment and use zero buffers to verify more behavior.
V8 Add DEBUG_VERBOSE message and unit test.
V7 addressed review comments (only documentation changes).

Current implementation of SafeString does not let one parse untrusted
data with its interfaces as they ASSERT on failing runtime checks and
require one to effectively reimplement the function on the caller side.

All the former proposals made it clear that the attempts to introduce
a solution preserving this behavior require a lot of changes
throughout the codebase including platform code (e.g. introducing
constraint assertions and updating all DebugLib implementations)
or require all sorts of hacks.

Since the original code has roughly no benefit except in some very
special cases and the effort required to preserve it is very high,
I propose to remove it as agreed on with several other parties.

Please note, that this patch does not change the behaviour of the
functions in RELEASE builds. I.e. they will still check for NULL
and return RETURN_INVALID_PARAMETER. In the future we may (or may
not) want them to simply ASSERT in this case. Regardless this should
be done in a separate BZ entry and a separate commit. For this reason
I ask everyone not to touch this subject.

Due to the amount of discussion this has already undergone after
almost a year I kindly request everyone to reduce the communication
to the minimum and abstain from proposing another approach.

Requesting to merge this into edk2-stable202005.

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: Michael D Kinney michael.d.kinney@intel.com
Cc: Vincent Zimmer vincent.zimmer@intel.com
Cc: Zhichao Gao zhichao.gao@intel.com
Cc: Jiewen Yao jiewen.yao@intel.com
Signed-off-by: Vitaly Cheptsov vit9696@protonmail.com

Michael D Kinney (1):
  MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK unit test

Vitaly Cheptsov (1):
  MdePkg: Fix SafeString performing assertions on runtime checks

 MdePkg/Include/Library/BaseLib.h              | 111 -----------------
 MdePkg/Library/BaseLib/SafeString.c           | 115 +-----------------
 .../UnitTest/Library/BaseLib/Base64UnitTest.c | 107 ++++++++++++++++
 3 files changed, 110 insertions(+), 223 deletions(-)

-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#59993): https://edk2.groups.io/g/devel/message/59993
Mute This Topic: https://groups.io/mt/74359140/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch v9 0/2] Disable safe string constraint assertions

Posted by Michael D Kinney 4 days ago
PR for V9 with all checks passing:

https://github.com/tianocore/edk2/pull/636

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Michael D Kinney
> Sent: Wednesday, May 20, 2020 1:10 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [Patch v9 0/2] Disable safe
> string constraint assertions
> 
> REF:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2054
> 
> V9 Update unit test comment and use zero buffers to
> verify more behavior.
> V8 Add DEBUG_VERBOSE message and unit test.
> V7 addressed review comments (only documentation
> changes).
> 
> Current implementation of SafeString does not let one
> parse untrusted
> data with its interfaces as they ASSERT on failing
> runtime checks and
> require one to effectively reimplement the function on
> the caller side.
> 
> All the former proposals made it clear that the
> attempts to introduce
> a solution preserving this behavior require a lot of
> changes
> throughout the codebase including platform code (e.g.
> introducing
> constraint assertions and updating all DebugLib
> implementations)
> or require all sorts of hacks.
> 
> Since the original code has roughly no benefit except
> in some very
> special cases and the effort required to preserve it is
> very high,
> I propose to remove it as agreed on with several other
> parties.
> 
> Please note, that this patch does not change the
> behaviour of the
> functions in RELEASE builds. I.e. they will still check
> for NULL
> and return RETURN_INVALID_PARAMETER. In the future we
> may (or may
> not) want them to simply ASSERT in this case.
> Regardless this should
> be done in a separate BZ entry and a separate commit.
> For this reason
> I ask everyone not to touch this subject.
> 
> Due to the amount of discussion this has already
> undergone after
> almost a year I kindly request everyone to reduce the
> communication
> to the minimum and abstain from proposing another
> approach.
> 
> Requesting to merge this into edk2-stable202005.
> 
> 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: Michael D Kinney michael.d.kinney@intel.com
> Cc: Vincent Zimmer vincent.zimmer@intel.com
> Cc: Zhichao Gao zhichao.gao@intel.com
> Cc: Jiewen Yao jiewen.yao@intel.com
> Signed-off-by: Vitaly Cheptsov vit9696@protonmail.com
> 
> Michael D Kinney (1):
>   MdePkg/Test/BaseLib: Add SAFE_STRING_CONSTRAINT_CHECK
> unit test
> 
> Vitaly Cheptsov (1):
>   MdePkg: Fix SafeString performing assertions on
> runtime checks
> 
>  MdePkg/Include/Library/BaseLib.h              | 111 --
> ---------------
>  MdePkg/Library/BaseLib/SafeString.c           | 115 +-
> ----------------
>  .../UnitTest/Library/BaseLib/Base64UnitTest.c | 107
> ++++++++++++++++
>  3 files changed, 110 insertions(+), 223 deletions(-)
> 
> --
> 2.21.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#59997): https://edk2.groups.io/g/devel/message/59997
Mute This Topic: https://groups.io/mt/74359140/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-