Re: [edk2-devel] [Patch v3] BaseTools: Fix GCC compiler failure in new added tools.

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

On Wed, Jul 10, 2019 at 01:42:44PM +0000, Liming Gao wrote:
> > > -  strncat (LocalStr, "[attributes] \n", sizeof("[attributes] \n"));
> > > +  strncat (LocalStr, "[attributes] \n", STR_LEN_MAX_4K - strlen (LocalStr) - 1);
> > 
> > This is a very inefficient, and difficult to read, way of doing this.
> > It also performs absolutely no error checking (making any future
> > debugging tedious at best).
> 
> This change is to fix gcc warning -Werror=sizeof-pointer-memaccess.
> strncat is the safe version API. So, it is used here.

Yes, but this code is continuously appending to a long string, and
then counting the length of that string. This could be done *very*
much more efficiently by keeping track of the current position we're
appending to and using strncpy. It would also improve readability.

As for the safe version - my point about error checking was not to do
with buffer overruns. It was to do with the fact that if the code ends
up filling the buffer, that will go undetected until something goes
wrong further along.

> > I have to be honest - looking at this code, I think the right thing to
> > do would be to revert the commit adding it and reworking the utility
> > completely (making sure it gets serious review) before merging it.
> > 
> > I am not surprised the compilers get upset.
> > 
> > This made me have a closer look at the patches adding FCE and FMMT,
> > and frankly, my opinion of them are similar. These aren't being
> > upstreamed - this is textbook "chuck over the wall".
> > 
> > Don't get me wrong - the code quality of FMMT is notably higher than
> > the code quality of FCE, which is notably higher than the code quality
> > of BfmLib. But even
> > 
> > BfmLib was added with the comment that it is used by FCE.
> > So why do FCE and FMMT both have separate copies of LibBfmGuidToStr?
> > Not just functions doing the same thing, but with the same name.
> > 
> > There are many other issues with these tools.
> 
> FCE & FMMT both operate the binary FV image. Their implementation
> may copy the same logic. I don't think this is a good way.
> I agree to do better design for code sharing and code quality improvement. 
> 
> I suggest to revert them, add current FCE & FMMT into edk2-staging,
> then refine them, and send the patch to add them back into edk2
> BaseTools.

This sounds good to me - thanks!

Best Regards,

Leif