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

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

[Ard Biesheuvel]

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>
---

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.
+      if (*Interface == NULL) {
+        //
+        // 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);
+
+        gBS->HandleProtocol (*Handle, Protocol, Interface);
+      }
     }
     DEBUG_CODE_BEGIN();
       if (EFI_ERROR(Status)) {
@@ -172,12 +179,6 @@ InitializeConsolePipe (
         }
       }
     DEBUG_CODE_END();
-
-    // If the console splitter driver is not supported by the platform then use the first Device Path
-    // instance for the console interface.
-    if (!EFI_ERROR(Status) && (*Interface == NULL)) {
-      Status = gBS->HandleProtocol (*Handle, Protocol, Interface);
-    }
   }
 
   // No Device Path has been defined for this console interface. We take the first protocol implementation
-- 
2.7.4

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

[Leif Lindholm]

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.

> +      if (*Interface == NULL) {
> +        //
> +        // 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);
> +
> +        gBS->HandleProtocol (*Handle, Protocol, Interface);
> +      }
>      }
>      DEBUG_CODE_BEGIN();
>        if (EFI_ERROR(Status)) {
> @@ -172,12 +179,6 @@ InitializeConsolePipe (
>          }
>        }
>      DEBUG_CODE_END();
> -
> -    // If the console splitter driver is not supported by the platform then use the first Device Path
> -    // instance for the console interface.
> -    if (!EFI_ERROR(Status) && (*Interface == NULL)) {
> -      Status = gBS->HandleProtocol (*Handle, Protocol, Interface);
> -    }
>    }
>  
>    // No Device Path has been defined for this console interface. We take the first protocol implementation
> -- 
> 2.7.4
> 

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

[Ard Biesheuvel]

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.

I will go ahead and apply this with a clarified comment.

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

[Ard Biesheuvel]

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.