Re: [PATCH edk2-platforms 5/5] Platform/RDKQemu: stop using deprecated string conversion routines

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Mon, Jun 10, 2019 at 08:55:03PM +0200, Ard Biesheuvel wrote:
> On Mon, 10 Jun 2019 at 20:06, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Mon, Jun 10, 2019 at 04:20:06PM +0200, Ard Biesheuvel wrote:
> > > Stop using deprecated string conversion routines so we can stop
> > > un'#define'ing the DISABLE_NEW_DEPRECATED_INTERFACES macro in this code.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  Platform/Comcast/Library/RdkBootManagerLib/DiskIo.c  | 12 +++++++--
> > >  Platform/Comcast/Library/RdkBootManagerLib/RdkFile.c | 28 +++++++++++---------
> > >  Platform/Comcast/RDKQemu/RDKQemu.dsc                 |  3 ---
> > >  3 files changed, 26 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/Platform/Comcast/Library/RdkBootManagerLib/DiskIo.c b/Platform/Comcast/Library/RdkBootManagerLib/DiskIo.c
> > > index ed893bd5af6a..df16c326cc57 100644
> > > --- a/Platform/Comcast/Library/RdkBootManagerLib/DiskIo.c
> > > +++ b/Platform/Comcast/Library/RdkBootManagerLib/DiskIo.c
> > > @@ -90,6 +90,7 @@ ListBlockIos (
> > >    UINTN                             NumHandles;
> > >    UINT16                            *DeviceFullPath;
> > >    DISKIO_PARTITION_LIST             *Entry;
> > > +  RETURN_STATUS                     RetStatus;
> > >
> > >    InitializeListHead (&mPartitionListHead);
> > >
> > > @@ -146,11 +147,13 @@ ListBlockIos (
> > >
> > >        // Copy handle and partition name
> > >        Entry->PartitionHandle = AllHandles[LoopIndex];
> > > -      StrnCpy (
> > > +      RetStatus = StrnCpyS (
> > >          Entry->PartitionName,
> > > +        PARTITION_NAME_MAX_LENGTH,
> > >          PartitionName,
> > >          PARTITION_NAME_MAX_LENGTH
> > >        );
> > > +      ASSERT_RETURN_ERROR (RetStatus);
> >
> > Would we not want to return an error here, for non-DEBUG builds?
> >
> 
> Actually, I think I should just change the last arg to
> PARTITION_NAME_MAX_LENGTH - 1, in which case no input length based
> error is ever returned.

Thats works for me.
With that, and the below style fixes:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> > >        InsertTailList (&mPartitionListHead, &Entry->Link);
> > >        break;
> > >      }
> > > @@ -176,8 +179,13 @@ OpenPartition (
> > >    DISKIO_PARTITION_LIST    *Entry;
> > >    SPARSE_HEADER            *SparseHeader;
> > >    UINT16                   UnicodePartitionName[100];
> > > +  RETURN_STATUS            RetStatus;
> > >
> > > -  AsciiStrToUnicodeStr ( PartitionName, UnicodePartitionName);
> > > +  RetStatus = AsciiStrToUnicodeStrS (PartitionName, UnicodePartitionName,
> > > +                sizeof (UnicodePartitionName));
> > > +  if (RETURN_ERROR (RetStatus)) {
> > > +    return EFI_OUT_OF_RESOURCES;
> > > +  }
> > >    DEBUG((DEBUG_INFO, "Unicode partition name %s\n", UnicodePartitionName));
> > >
> > >    Status = ListBlockIos (UnicodePartitionName);
> > > diff --git a/Platform/Comcast/Library/RdkBootManagerLib/RdkFile.c b/Platform/Comcast/Library/RdkBootManagerLib/RdkFile.c
> > > index dd695651026a..6f98562eff84 100644
> > > --- a/Platform/Comcast/Library/RdkBootManagerLib/RdkFile.c
> > > +++ b/Platform/Comcast/Library/RdkBootManagerLib/RdkFile.c
> > > @@ -8,8 +8,6 @@
> > >
> > >  #define MAX_VAR       6
> > >
> > > -#define ALLOCATE_STRING_MEM(X)  AllocateZeroPool((X + 1) * sizeof(CHAR16))
> > > -
> > >  /**
> > >   * list_for_each_entry       -       iterate over list of given type
> > >   * @pos:     the type * to use as a loop cursor.
> > > @@ -53,10 +51,13 @@ SaveString (
> > >    IN  CHAR16    *String2
> > >    )
> > >  {
> > > -  *Dest = ALLOCATE_STRING_MEM (StrLen (String1) + StrLen (String2));
> > > +  UINTN StringLength;
> > > +
> > > +  StringLength = StrLen (String1) + StrLen (String2) + 1;
> > > +  *Dest = AllocatePool (StringLength * sizeof(CHAR16));
> >
> > Minor, and just part of the shuffle, but - space after sizeof?
> > (At least one more below.)
> >
> 
> Right, will fix.
> 
> > >    ASSERT (Dest != NULL);
> > > -  StrCat (*Dest, String1);
> > > -  StrCat (*Dest, String2);
> > > +  StrCpyS (*Dest, StringLength, String1);
> > > +  StrCatS (*Dest, StringLength, String2);
> > >  }
> > >
> > >  STATIC
> > > @@ -74,11 +75,13 @@ LsFiles (
> > >    BOOLEAN             NoFile;
> > >    CHAR16              *TempPath;
> > >    DIR_NODE            *Node;
> > > +  UINTN               StringLength;
> > >
> > >    NoFile    = FALSE;
> > > -  TempPath  = ALLOCATE_STRING_MEM (StrLen(DirPath) + 1);
> > > -  StrCat (TempPath, DirPath);
> > > -  StrCat (TempPath, L"/");
> > > +  StringLength = StrLen(DirPath) + 2;
> > > +  TempPath = AllocatePool (StringLength * sizeof(CHAR16));
> > > +  StrCpyS (TempPath, StringLength, DirPath);
> > > +  StrCatS (TempPath, StringLength, L"/");
> > >
> > >    Status = GetFileHandler (&FileHandle, DirPath, EFI_FILE_MODE_READ);
> > >    ASSERT_EFI_ERROR (Status);
> > > @@ -192,6 +195,7 @@ InitVarList (
> > >    UINTN       Next;
> > >    CHAR8       *VarDelimiter[2];
> > >    EFI_STATUS  Status;
> > > +  UINTN       StringLength;
> > >
> > >    VarDelimiter[0] = "=";
> > >    VarDelimiter[1] = "\"";
> > > @@ -212,10 +216,10 @@ InitVarList (
> > >        if (VarResult[OuterLoopIndex][InnerLoopIndex]) {
> > >          FreePool (VarResult[OuterLoopIndex][InnerLoopIndex]);
> > >        }
> > > -      VarResult[OuterLoopIndex][InnerLoopIndex] = \
> > > -        ALLOCATE_STRING_MEM (AsciiStrLen (&FileData[Current]));
> > > -      AsciiStrToUnicodeStr (&FileData[Current], \
> > > -        VarResult[OuterLoopIndex][InnerLoopIndex]);
> > > +      StringLength = AsciiStrLen (&FileData[Current]) + 1;
> > > +      VarResult[OuterLoopIndex][InnerLoopIndex] = AllocatePool (StringLength);
> > > +      AsciiStrToUnicodeStrS (&FileData[Current],
> > > +        VarResult[OuterLoopIndex][InnerLoopIndex], StringLength);
> > >        //skip new line
> > >        Next += 2;
> > >      }
> > > diff --git a/Platform/Comcast/RDKQemu/RDKQemu.dsc b/Platform/Comcast/RDKQemu/RDKQemu.dsc
> > > index f0ed4f11e81d..440d2ace917c 100644
> > > --- a/Platform/Comcast/RDKQemu/RDKQemu.dsc
> > > +++ b/Platform/Comcast/RDKQemu/RDKQemu.dsc
> > > @@ -418,6 +418,3 @@ [Components.AARCH64]
> > >      <LibraryClasses>
> > >        NULL|ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
> > >    }
> > > -
> > > -[BuildOptions]
> > > -  GCC:*_*_*_CC_FLAGS = -UDISABLE_NEW_DEPRECATED_INTERFACES
> > > --
> > > 2.20.1
> > >