public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Wang, Jian J" <jian.j.wang@intel.com>
To: "Carsey, Jaben" <jaben.carsey@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>, "Bi, Dandan" <dandan.bi@intel.com>
Subject: Re: [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool
Date: Thu, 9 Nov 2017 01:03:15 +0000	[thread overview]
Message-ID: <D827630B58408649ACB04F44C510003624CAA85B@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <CB6E33457884FA40993F35157061515CA3BCAAA6@FMSMSX103.amr.corp.intel.com>

You're right StrCpyS is better for this case. Since the patch has checked in,
maybe we can refine those code in another patch. Thanks for the comment.

> -----Original Message-----
> From: Carsey, Jaben
> Sent: Wednesday, November 08, 2017 11:50 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: RE: [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool
> 
> Why not use the StrCpy_s function to copy strings?  CopyMem and StrSize feels
> odd to me.
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Tuesday, November 07, 2017 6:12 PM
> > To: edk2-devel@lists.01.org
> > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu
> > <ruiyu.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>
> > Subject: [PATCH v3 2/3] ShellPkg: Fix misuses of AllocateCopyPool
> > Importance: High
> >
> > > v3:
> > >   No update
> >
> > > v2:
> > > a. Use ReallocatePool instead of allocating then copying wherever
> > applicable
> >
> > AllocateCopyPool(AllocationSize, *Buffer) will copy "AllocationSize" bytes of
> > memory from old "Buffer" to new allocated one. If "AllocationSize" is bigger
> > than size of "Buffer", heap memory overflow occurs during copy.
> >
> > One solution is to allocate pool first then copy the necessary bytes to new
> > memory. Another is using ReallocatePool instead if old buffer will be freed
> > on spot.
> >
> > Cc: Jaben Carsey <jaben.carsey@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Bi Dandan <dandan.bi@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  ShellPkg/Application/Shell/Shell.c                                 | 4 +++-
> >  ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 7
> > +++++--
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/ShellPkg/Application/Shell/Shell.c
> > b/ShellPkg/Application/Shell/Shell.c
> > index 5471930ba1..656206fdce 100644
> > --- a/ShellPkg/Application/Shell/Shell.c
> > +++ b/ShellPkg/Application/Shell/Shell.c
> > @@ -1646,7 +1646,7 @@ ShellConvertVariables (
> >    //
> >    // now do the replacements...
> >    //
> > -  NewCommandLine1 = AllocateCopyPool(NewSize, OriginalCommandLine);
> > +  NewCommandLine1 = AllocateZeroPool (NewSize);
> >    NewCommandLine2 = AllocateZeroPool(NewSize);
> >    ItemTemp        = AllocateZeroPool(ItemSize+(2*sizeof(CHAR16)));
> >    if (NewCommandLine1 == NULL || NewCommandLine2 == NULL ||
> > ItemTemp == NULL) {
> > @@ -1655,6 +1655,8 @@ ShellConvertVariables (
> >      SHELL_FREE_NON_NULL(ItemTemp);
> >      return (NULL);
> >    }
> > +  CopyMem (NewCommandLine1, OriginalCommandLine, StrSize
> > (OriginalCommandLine));
> > +
> >    for (MasterEnvList = EfiShellGetEnv(NULL)
> >      ;  MasterEnvList != NULL && *MasterEnvList != CHAR_NULL
> >      ;  MasterEnvList += StrLen(MasterEnvList) + 1
> > diff --git
> > a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> > b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> > index 1122c89b8b..ee3db63358 100644
> > ---
> > a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> > +++
> > b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> > @@ -143,10 +143,11 @@ UpdateOptionalData(
> >      OriginalOptionDataSize += (*(UINT16*)(OriginalData + sizeof(UINT32)));
> >      OriginalOptionDataSize -= OriginalSize;
> >      NewSize = OriginalSize - OriginalOptionDataSize + DataSize;
> > -    NewData = AllocateCopyPool(NewSize, OriginalData);
> > +    NewData = AllocatePool(NewSize);
> >      if (NewData == NULL) {
> >        Status = EFI_OUT_OF_RESOURCES;
> >      } else {
> > +      CopyMem (NewData, OriginalData, OriginalSize -
> > OriginalOptionDataSize);
> >        CopyMem(NewData + OriginalSize - OriginalOptionDataSize, Data,
> > DataSize);
> >      }
> >    }
> > @@ -1120,11 +1121,13 @@ BcfgAddOpt(
> >          // Now we know how many EFI_INPUT_KEY structs we need to attach to
> > the end of the EFI_KEY_OPTION struct.
> >          // Re-allocate with the added information.
> >          //
> > -        KeyOptionBuffer = AllocateCopyPool(sizeof(EFI_KEY_OPTION) +
> > (sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount),
> > &NewKeyOption);
> > +        KeyOptionBuffer = AllocatePool (sizeof(EFI_KEY_OPTION) +
> > (sizeof(EFI_INPUT_KEY) *
> > NewKeyOption.KeyData.Options.InputKeyCount));
> >          if (KeyOptionBuffer == NULL) {
> >            ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_MEM),
> > gShellBcfgHiiHandle, L"bcfg");
> >            ShellStatus = SHELL_OUT_OF_RESOURCES;
> > +          return ShellStatus;
> >          }
> > +        CopyMem (KeyOptionBuffer, &NewKeyOption,
> > sizeof(EFI_KEY_OPTION));
> >        }
> >        for (LoopCounter = 0 ; ShellStatus == SHELL_SUCCESS && LoopCounter <
> > NewKeyOption.KeyData.Options.InputKeyCount; LoopCounter++) {
> >          //
> > --
> > 2.14.1.windows.1



  reply	other threads:[~2017-11-09  0:59 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-08  2:11 [PATCH v3 0/3] Fix misuses of AllocateCopyPool Jian J Wang
2017-11-08  2:11 ` [PATCH v3 1/3] MdeModulePkg: " Jian J Wang
2017-11-08  2:37   ` Zeng, Star
2017-11-08  2:46     ` Wang, Jian J
2017-11-08  2:57       ` Zeng, Star
2017-11-08  2:12 ` [PATCH v3 2/3] ShellPkg: " Jian J Wang
2017-11-08 15:50   ` Carsey, Jaben
2017-11-09  1:03     ` Wang, Jian J [this message]
2017-11-08  2:12 ` [PATCH v3 3/3] IntelFrameworkModulePkg: " Jian J Wang
2017-11-08  2:33   ` Bi, Dandan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=D827630B58408649ACB04F44C510003624CAA85B@SHSMSX103.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox