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

[Laszlo Ersek <lersek@redhat.com>]

On 10/24/16 09:57, Ard Biesheuvel wrote:
> 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.
> 

Okay, will do. :)