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
