Re: [PATCH] ArmPlatformPkg/PlatformIntelBdsLib: don't clobber ConSplitter handle

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

On 3 March 2017 at 21:32, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 3 March 2017 at 21:23, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Wed, Mar 01, 2017 at 06:34:33PM +0000, Ard Biesheuvel wrote:
>>> The InitializeConsolePipe() routine takes care to only set its output
>>> argument *Interface if it is not already set, to prevent overwriting
>>> the ConSplitter interface pointer that may have already been assigned.
>>>
>>> However, the associated OUT argument 'Handle' is clobbered by the
>>> subsequent unnecessary LocateDevicePath() invocation, which should
>>> similarly be made dependent on whether *Interface has been set
>>> already.
>>>
>>> Reported-by: "Lee, Terry Ping-Chung" <terry.lee@hpe.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> This looks good. I'll make one non-functional ecomment, and you can
>> decide whether to incorporate it before pushing or not.
>>
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>>> ---
>>>
>>> Terry: does this work for you? Thanks.
>>>
>>>  ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c | 25 ++++++++++----------
>>>  1 file changed, 13 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>>> index 885866854329..f8e04efea63d 100644
>>> --- a/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>>> +++ b/ArmPlatformPkg/Library/PlatformIntelBdsLib/IntelBdsPlatform.c
>>> @@ -148,12 +148,19 @@ InitializeConsolePipe (
>>>
>>>      Status = BdsLibConnectDevicePath (DevicePath);
>>>      if (!EFI_ERROR (Status)) {
>>> -      //
>>> -      // If BdsLibConnectDevicePath () succeeded, *Handle must have a non-NULL
>>> -      // value. So ASSERT that this is the case.
>>> -      //
>>> -      gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid, &DevicePath, Handle);
>>> -      ASSERT (*Handle != NULL);
>>> +
>>> +      // If the console splitter driver is not supported by the platform then
>>> +      // use the first Device Path instance for the console interface.
>>
>> I realise this comment is just being moved, but it's confusing. It
>> refers to a state that is only visible if you're already aware of 2 or
>> 3 facts not visible at this point.
>>
>> If I understand the situation correctly, the following would be a more
>> helpful description:
>>
>> // If the console splitter driver is supported by the platform, then
>> // *Interface is already initialized. If it is not, use the first
>> // successfully connected device as the console.
>>
>
> Not entirely. The consplitter is one example of where multiple
> consoles may be available, but it really applies to any situation
> where multiple device paths are provided. So it is not that the
> function is *entered* with *Interface non-null, but that any
> additional iterations of the loop will clobber *Handle whereas
> *Interface will only be set in the first iteration.
>

Actually, I suppose it may be entered with *Interface non-NULL, but
even if it is NULL we should take care to update *Handle and
*interface at the same time only.