Re: [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with AsciiStrCatS()

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> AsciiStrCat() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> The caller of CpsrString() is required to pass in "ReturnStr" with 32
> CHAR8 elements. (DefaultExceptionHandler() complies with this.) "Str" is
> used to build "ReturnStr" gradually. Just before calling AsciiStrCat(),
> "Str" points to the then-terminating NUL character in "ReturnStr".
>
> The difference (Str - ReturnStr) gives the number of non-NUL characters
> we've written thus far, hence (32 - (Str - ReturnStr)) yields the number
> of remaining bytes in ReturnStr, including the ultimately terminating NUL
> character.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     - build-tested only
>
>     - Michael (CC'd) had posted a patch earlier for this:
>       <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
>       but I only noticed that now that he pointed it out, in
>       <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
>       I'll leave it to the ArmPkg maintainers to choose one; I don't feel
>       strongly either way.
>
>  ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> index aece26355e2e..4b2ee9a9acf7 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> @@ -116,7 +116,10 @@ CpsrString (
>      break;
>    }
>
> -  AsciiStrCat (Str, ModeStr);
> +  //
> +  // See the interface contract in the leading comment block.
> +  //
> +  AsciiStrCatS (Str, 32 - (Str - ReturnStr), ModeStr);
>    return;

Could you please use a symbolic constant for this '32', and replace it
in the declaration of CpsrStr[] as well?

With that

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Oh, and if the bogus 'return' happens to get lost along the way as
well, I would not mind.