public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <leif.lindholm@linaro.org>
To: devel@edk2.groups.io, liming.gao@intel.com
Cc: "Gao, Zhichao" <zhichao.gao@intel.com>,
	"Feng, Bob C" <bob.c.feng@intel.com>,
	Andrew Fish <afish@apple.com>, Laszlo Ersek <lersek@redhat.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [Patch v3] BaseTools: Fix GCC compiler failure in new added tools.
Date: Wed, 10 Jul 2019 21:57:11 +0100	[thread overview]
Message-ID: <20190710205711.wqqzbvscbt4war6h@bivouac.eciton.net> (raw)
In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E4A53C2@SHSMSX104.ccr.corp.intel.com>

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

      parent reply	other threads:[~2019-07-10 20:57 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-09  9:53 [Patch v3] BaseTools: Fix GCC compiler failure in new added tools Liming Gao
2019-07-09 10:19 ` [edk2-devel] " Gary Lin
2019-07-09 18:26 ` Leif Lindholm
2019-07-10 13:42   ` Liming Gao
2019-07-10 20:10     ` Michael D Kinney
2019-07-11  1:47       ` Liming Gao
2019-07-10 20:57     ` Leif Lindholm [this message]

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=20190710205711.wqqzbvscbt4war6h@bivouac.eciton.net \
    --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