This patch enhances error handling and reporting in the CM ObjectParser.
Specifically:
1. ObjectIDs used as array indexes are checked for being out of bounds,
   and if so an error message is printed before the assert.
2. An error message is printed for unsupported NameSpaceIDs.
3. Adds support for unimplemented parsers by allowing IDs to list a
   NULL parser, resulting in an unimplemented message being printed.

Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
---
 .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
index 92df1efee8..22b8fdb906 100644
--- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
@@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
     ARRAY_SIZE (StdObjAcpiTableInfoParser) },
   { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
     ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
+  { "EStdObjMax",             NULL,                       0}
 };
 
 /** Print string data.
@@ -1066,6 +1067,12 @@ ParseCmObjDesc (
         return;
       }
 
+      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
+        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
+        ASSERT (0);
+        return;
+      }
+
       ParserArray = &StdNamespaceObjectParser[ObjId];
       break;
     case EObjNameSpaceArm:
@@ -1074,10 +1081,17 @@ ParseCmObjDesc (
         return;
       }
 
+      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
+        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
+        ASSERT (0);
+        return;
+      }
+
       ParserArray = &ArmNamespaceObjectParser[ObjId];
       break;
     default:
       // Not supported
+      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not supported by the parser\n", NameSpaceId, ObjId));
       ASSERT (0);
       return;
   } // switch
@@ -1095,21 +1109,26 @@ ParseCmObjDesc (
       ObjIndex + 1,
       ObjectCount
       ));
-    PrintCmObjDesc (
-      (VOID *)((UINTN)CmObjDesc->Data + Offset),
-      ParserArray->Parser,
-      ParserArray->ItemCount,
-      &RemainingSize,
-      1
-      );
-    if ((RemainingSize > CmObjDesc->Size) ||
-        (RemainingSize < 0))
-    {
-      ASSERT (0);
-      return;
-    }
+    if (ParserArray->Parser == NULL) {
+      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
+      RemainingSize = 0;
+    } else {
+      PrintCmObjDesc (
+        (VOID *)((UINTN)CmObjDesc->Data + Offset),
+        ParserArray->Parser,
+        ParserArray->ItemCount,
+        &RemainingSize,
+        1
+        );
+      if ((RemainingSize > CmObjDesc->Size) ||
+          (RemainingSize < 0))
+      {
+        ASSERT (0);
+        return;
+      }
 
-    Offset = CmObjDesc->Size - RemainingSize;
+      Offset = CmObjDesc->Size - RemainingSize;
+    }
   } // for
 
   ASSERT (RemainingSize == 0);
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109364): https://edk2.groups.io/g/devel/message/109364
Mute This Topic: https://groups.io/mt/101801385/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hello Jeshua,

On 10/6/23 18:28, Jeshua Smith wrote:
> This patch enhances error handling and reporting in the CM ObjectParser.
> Specifically:
> 1. ObjectIDs used as array indexes are checked for being out of bounds,
>     and if so an error message is printed before the assert.
> 2. An error message is printed for unsupported NameSpaceIDs.
> 3. Adds support for unimplemented parsers by allowing IDs to list a
>     NULL parser, resulting in an unimplemented message being printed.

I am not sure I see in which context 3. would be used/necessary. Is it possible
to detail ?

(Code-wise everything looks good to me)

Regards,
Pierre


> 
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> ---
>   .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
>   1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> index 92df1efee8..22b8fdb906 100644
> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>       ARRAY_SIZE (StdObjAcpiTableInfoParser) },
>     { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
>       ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
> +  { "EStdObjMax",             NULL,                       0}
>   };
>   
>   /** Print string data.
> @@ -1066,6 +1067,12 @@ ParseCmObjDesc (
>           return;
>         }
>   
> +      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &StdNamespaceObjectParser[ObjId];
>         break;
>       case EObjNameSpaceArm:
> @@ -1074,10 +1081,17 @@ ParseCmObjDesc (
>           return;
>         }
>   
> +      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &ArmNamespaceObjectParser[ObjId];
>         break;
>       default:
>         // Not supported
> +      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not supported by the parser\n", NameSpaceId, ObjId));
>         ASSERT (0);
>         return;
>     } // switch
> @@ -1095,21 +1109,26 @@ ParseCmObjDesc (
>         ObjIndex + 1,
>         ObjectCount
>         ));
> -    PrintCmObjDesc (
> -      (VOID *)((UINTN)CmObjDesc->Data + Offset),
> -      ParserArray->Parser,
> -      ParserArray->ItemCount,
> -      &RemainingSize,
> -      1
> -      );
> -    if ((RemainingSize > CmObjDesc->Size) ||
> -        (RemainingSize < 0))
> -    {
> -      ASSERT (0);
> -      return;
> -    }
> +    if (ParserArray->Parser == NULL) {
> +      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
> +      RemainingSize = 0;
> +    } else {
> +      PrintCmObjDesc (
> +        (VOID *)((UINTN)CmObjDesc->Data + Offset),
> +        ParserArray->Parser,
> +        ParserArray->ItemCount,
> +        &RemainingSize,
> +        1
> +        );
> +      if ((RemainingSize > CmObjDesc->Size) ||
> +          (RemainingSize < 0))
> +      {
> +        ASSERT (0);
> +        return;
> +      }
>   
> -    Offset = CmObjDesc->Size - RemainingSize;
> +      Offset = CmObjDesc->Size - RemainingSize;
> +    }
>     } // for
>   
>     ASSERT (RemainingSize == 0);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109486): https://edk2.groups.io/g/devel/message/109486
Mute This Topic: https://groups.io/mt/101801385/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

#3 is not currently used by any published code. It is a development aid, which the ObjectParser itself seems to be.

Here's why I added it. Several people on our team have (not yet upstreamed) changes that resulted in additional ObjectIDs being added to the ObjectID enums, but without corresponding parsers being added to the ObjectParser. Dumping of the objects with the ObjectParser wasn't enabled by default, so this wasn't detected by them. Without #1 the ObjectParser code does out of bounds array accesses, sometimes leading to crashes. #1 will now detect and report that problem. When I enabled dumping of objects to debug my new code, I hit this bug leading me to write #1. For me to work around the issue of missing parsers in order to be able to continue debug with my work, the "easy" solution was for me to temporarily add the new ObjectIDs to the parser list with NULL parsers and then inform the responsible parties that they need to go and add parsers for their new ObjectIDs. Doing this required #3 (support for NULL parsers) to be added. Ideally any code that is upstreamed back to EDKII will have non-NULL parsers at the point it is sent upstream, but allowing in-development changes to temporarily use NULL parsers is helpful.

Hopefully that clarifies things.

-----Original Message-----
From: Pierre Gondois <pierre.gondois@arm.com> 
Sent: Tuesday, October 10, 2023 4:14 AM
To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com
Subject: Re: [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

External email: Use caution opening links or attachments


Hello Jeshua,

On 10/6/23 18:28, Jeshua Smith wrote:
> This patch enhances error handling and reporting in the CM ObjectParser.
> Specifically:
> 1. ObjectIDs used as array indexes are checked for being out of bounds,
>     and if so an error message is printed before the assert.
> 2. An error message is printed for unsupported NameSpaceIDs.
> 3. Adds support for unimplemented parsers by allowing IDs to list a
>     NULL parser, resulting in an unimplemented message being printed.

I am not sure I see in which context 3. would be used/necessary. Is it possible to detail ?

(Code-wise everything looks good to me)

Regards,
Pierre


>
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> ---
>   .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
>   1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c 
> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> index 92df1efee8..22b8fdb906 100644
> --- 
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana
> +++ gerObjectParser.c
> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>       ARRAY_SIZE (StdObjAcpiTableInfoParser) },
>     { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
>       ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
> +  { "EStdObjMax",             NULL,                       0}
>   };
>
>   /** Print string data.
> @@ -1066,6 +1067,12 @@ ParseCmObjDesc (
>           return;
>         }
>
> +      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &StdNamespaceObjectParser[ObjId];
>         break;
>       case EObjNameSpaceArm:
> @@ -1074,10 +1081,17 @@ ParseCmObjDesc (
>           return;
>         }
>
> +      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &ArmNamespaceObjectParser[ObjId];
>         break;
>       default:
>         // Not supported
> +      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not 
> + supported by the parser\n", NameSpaceId, ObjId));
>         ASSERT (0);
>         return;
>     } // switch
> @@ -1095,21 +1109,26 @@ ParseCmObjDesc (
>         ObjIndex + 1,
>         ObjectCount
>         ));
> -    PrintCmObjDesc (
> -      (VOID *)((UINTN)CmObjDesc->Data + Offset),
> -      ParserArray->Parser,
> -      ParserArray->ItemCount,
> -      &RemainingSize,
> -      1
> -      );
> -    if ((RemainingSize > CmObjDesc->Size) ||
> -        (RemainingSize < 0))
> -    {
> -      ASSERT (0);
> -      return;
> -    }
> +    if (ParserArray->Parser == NULL) {
> +      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
> +      RemainingSize = 0;
> +    } else {
> +      PrintCmObjDesc (
> +        (VOID *)((UINTN)CmObjDesc->Data + Offset),
> +        ParserArray->Parser,
> +        ParserArray->ItemCount,
> +        &RemainingSize,
> +        1
> +        );
> +      if ((RemainingSize > CmObjDesc->Size) ||
> +          (RemainingSize < 0))
> +      {
> +        ASSERT (0);
> +        return;
> +      }
>
> -    Offset = CmObjDesc->Size - RemainingSize;
> +      Offset = CmObjDesc->Size - RemainingSize;
> +    }
>     } // for
>
>     ASSERT (RemainingSize == 0);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109499): https://edk2.groups.io/g/devel/message/109499
Mute This Topic: https://groups.io/mt/101801385/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Jeshua,

On 10/10/23 17:41, Jeshua Smith wrote:
> #3 is not currently used by any published code. It is a development aid, which the ObjectParser itself seems to be.
> 
> Here's why I added it. Several people on our team have (not yet upstreamed) changes that resulted in additional ObjectIDs being added to the ObjectID enums, but without corresponding parsers being added to the ObjectParser. Dumping of the objects with the ObjectParser wasn't enabled by default, so this wasn't detected by them. Without #1 the ObjectParser code does out of bounds array accesses, sometimes leading to crashes. #1 will now detect and report that problem. When I enabled dumping of objects to debug my new code, I hit this bug leading me to write #1. For me to work around the issue of missing parsers in order to be able to continue debug with my work, the "easy" solution was for me to temporarily add the new ObjectIDs to the parser list with NULL parsers and then inform the responsible parties that they need to go and add parsers for their new ObjectIDs. Doing this required #3 (support for NULL parsers) to be added. Ideally any code that is upstreamed back to EDKII will have non-NULL parsers at the point it is sent upstream, but allowing in-development changes to temporarily use NULL parsers is helpful.
> 
> Hopefully that clarifies things.

Thanks for the detailed explanation,
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

> 
> -----Original Message-----
> From: Pierre Gondois <pierre.gondois@arm.com>
> Sent: Tuesday, October 10, 2023 4:14 AM
> To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io
> Cc: Sami.Mujawar@arm.com
> Subject: Re: [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling
> 
> External email: Use caution opening links or attachments
> 
> 
> Hello Jeshua,
> 
> On 10/6/23 18:28, Jeshua Smith wrote:
>> This patch enhances error handling and reporting in the CM ObjectParser.
>> Specifically:
>> 1. ObjectIDs used as array indexes are checked for being out of bounds,
>>      and if so an error message is printed before the assert.
>> 2. An error message is printed for unsupported NameSpaceIDs.
>> 3. Adds support for unimplemented parsers by allowing IDs to list a
>>      NULL parser, resulting in an unimplemented message being printed.
> 
> I am not sure I see in which context 3. would be used/necessary. Is it possible to detail ?
> 
> (Code-wise everything looks good to me)
> 
> Regards,
> Pierre
> 
> 
>>
>> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
>> ---
>>    .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
>>    1 file changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git
>> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
>> bjectParser.c
>> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
>> bjectParser.c
>> index 92df1efee8..22b8fdb906 100644
>> ---
>> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
>> bjectParser.c
>> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana
>> +++ gerObjectParser.c
>> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>>        ARRAY_SIZE (StdObjAcpiTableInfoParser) },
>>      { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
>>        ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
>> +  { "EStdObjMax",             NULL,                       0}
>>    };
>>
>>    /** Print string data.
>> @@ -1066,6 +1067,12 @@ ParseCmObjDesc (
>>            return;
>>          }
>>
>> +      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
>> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
>> +        ASSERT (0);
>> +        return;
>> +      }
>> +
>>          ParserArray = &StdNamespaceObjectParser[ObjId];
>>          break;
>>        case EObjNameSpaceArm:
>> @@ -1074,10 +1081,17 @@ ParseCmObjDesc (
>>            return;
>>          }
>>
>> +      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
>> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
>> +        ASSERT (0);
>> +        return;
>> +      }
>> +
>>          ParserArray = &ArmNamespaceObjectParser[ObjId];
>>          break;
>>        default:
>>          // Not supported
>> +      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not
>> + supported by the parser\n", NameSpaceId, ObjId));
>>          ASSERT (0);
>>          return;
>>      } // switch
>> @@ -1095,21 +1109,26 @@ ParseCmObjDesc (
>>          ObjIndex + 1,
>>          ObjectCount
>>          ));
>> -    PrintCmObjDesc (
>> -      (VOID *)((UINTN)CmObjDesc->Data + Offset),
>> -      ParserArray->Parser,
>> -      ParserArray->ItemCount,
>> -      &RemainingSize,
>> -      1
>> -      );
>> -    if ((RemainingSize > CmObjDesc->Size) ||
>> -        (RemainingSize < 0))
>> -    {
>> -      ASSERT (0);
>> -      return;
>> -    }
>> +    if (ParserArray->Parser == NULL) {
>> +      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
>> +      RemainingSize = 0;
>> +    } else {
>> +      PrintCmObjDesc (
>> +        (VOID *)((UINTN)CmObjDesc->Data + Offset),
>> +        ParserArray->Parser,
>> +        ParserArray->ItemCount,
>> +        &RemainingSize,
>> +        1
>> +        );
>> +      if ((RemainingSize > CmObjDesc->Size) ||
>> +          (RemainingSize < 0))
>> +      {
>> +        ASSERT (0);
>> +        return;
>> +      }
>>
>> -    Offset = CmObjDesc->Size - RemainingSize;
>> +      Offset = CmObjDesc->Size - RemainingSize;
>> +    }
>>      } // for
>>
>>      ASSERT (RemainingSize == 0);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109524): https://edk2.groups.io/g/devel/message/109524
Mute This Topic: https://groups.io/mt/101801385/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-