Hi, Martin
My intention is initializing the FsInfoOut with the destination size and the
VolumeLable field of FsInfoOut got updated at next statement (StrCpy operation).
CopyMem (FsInfoOut, &mFsInfoTemplate, Size);
StrCpy (FsInfoOut->VolumeLabel, Instance->VolumeLabel);
But anyway it's not a good coding style and I will update it to use source
buffer size.
PS: I will also update all Str*() to use latest SafeString APIs in BaseLib for
security intention.
Thanks
Feng
-----Original Message-----
From: Olivier Martin [mailto:olivier.mar...@arm.com]
Sent: Thursday, November 13, 2014 07:32
To: edk2-devel@lists.sourceforge.net; Tian, Feng
Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem module
I do not disagree but can we assume Instance->VolumeLabel will always be an
empty string?
My point is 'Instance->VolumeLabel' can contain a valid non empty string.
It means:
Size = sizeof (EFI_FILE_SYSTEM_INFO) + StrSize (Instance->VolumeLabel) - sizeof
(CHAR16) > sizeof (EFI_FILE_SYSTEM_INFO)
So when we do:
CopyMem (FsInfoOut, &mFsInfoTemplate, Size); We might copy some additional
bytes after mFsInfoTemplate declaration.
________________________________________
From: Carsey, Jaben [jaben.car...@intel.com]
Sent: 12 November 2014 16:15
To: edk2-devel@lists.sourceforge.net; Tian, Feng
Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem module
With an empty string isn't StrSize going to return 2, and then sizeof(CHAR16)
also returning 2, so the math will result in the same copy as the original?
-Jaben
> -----Original Message-----
> From: Olivier Martin [mailto:olivier.mar...@arm.com]
> Sent: Wednesday, November 12, 2014 3:35 AM
> To: Tian, Feng
> Cc: edk2-devel@lists.sourceforge.net
> Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem
> module
>
> Thanks a lot for submitted a revised version.
> There is nothing controversial in your changes. Except maybe these
> lines in
> FvSimpleFileSystemGetInfo():
>
> Size = sizeof (EFI_FILE_SYSTEM_INFO) + StrSize
> (Instance->VolumeLabel) - sizeof (CHAR16);
> CopyMem (FsInfoOut, &mFsInfoTemplate, Size);
>
> While it was in the original version:
>
> CopyMem (FsInfoOut, &mFsInfoTemplate,
> sizeof(EFI_FILE_SYSTEM_INFO));
>
>
> With mFsInfoTemplate:
>
> EFI_FILE_SYSTEM_INFO mFsInfoTemplate = {
> 0, // Populate at runtime
> TRUE, // Read-only
> 0, // Don't know volume size
> 0, // No free space
> 0, // Don't know block size
> L"" // Populate at runtime
> };
>
> It means you are copying more bytes than the actual source variable.
>
>
> > -----Original Message-----
> > From: Tian, Feng [mailto:feng.t...@intel.com]
> > Sent: 12 November 2014 07:16
> > To: Olivier Martin
> > Cc: edk2-devel@lists.sourceforge.net; Tian, Feng
> > Subject: [edk2] patch review feedback about FvSimpleFileSystem
> > module
> >
> > Hi, Martin
> >
> > Here is my review comments on your FvSimpleFileSystem uefi driver.
> > Sorry for late response.
> >
> > 1. Fixed system hang due to wrong assertions in
> > FvSimpleFileSystemDriverStart() and FvSimpleFileSystemOpenVolume().
> > a. change ASSERT (NumChars == GUID_STRING_SIZE); to ASSERT
> > ((NumChars + 1) * sizeof (CHAR16) == GUID_STRING_SIZE);
> > b. change ASSERT (NumChars == FVFS_VOLUME_LABEL_SIZE); to
> > ASSERT ((NumChars + 1) * sizeof (CHAR16) ==
> FVFS_VOLUME_LABEL_SIZE);
> > 2. Fixed SimpleFileSystem SCT errors.
> > a. EFI_FILE_PROTOCOL.GetInfo() should handle VolumeLabel
> > info rather than return EFI_UNSUPPORTED.
> > b. EFI_FILE_PROTOCOL.GetInfo() should initial Size field of
> > EFI_FILE_SYSTEM_INFO at run time.
> > c. Some interfaces in EFI_FILE_PROTOCOL should return
> > EFI_WRITE_PROTECTED rather than EFI_UNSUPPORTED.
> > d. EFI_FILE_PROTOCOL.Delete() should return
> > EFI_WARN_DELETE_FAILURE rather than EFI_UNSUPPORTED.
> > 3. Added missing ComponentName protocol interface and modify code
> > to use APIs defined in UefiLib to install DriverBinding and
> > ComponentName protocol.
> > 4. Fixed wrong UnicodeCollation protocol usage.
> > 5. Added missing function comments.
> > 6. Added missing user extension meta data in INF file.
> > 7. Remove BasePathLib dependency and use internal implementation as
> > the move operation for this library and others in ShellPkg need more
> > discussions.
> >
> > I attached the modified path for your review, will check it in to
> > MdeModulePkg/Universal after pass your review. BasePathLib will be
> > on other thread.
> >
> > Thanks
> > Feng
>
>
>
>
> ----------------------------------------------------------------------
> -------- Comprehensive Server Monitoring with Site24x7.
> Monitor 10 servers for $9/Month.
> Get alerted through email, SMS, voice calls or mobile push notifications.
> Take corrective actions from your mobile device.
> http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.
> cl
> ktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
-- IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered
in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office
110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company
No: 2548782
------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
