From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yann E. MORIN <yann.morin.1998@free.fr>
Date: Tue, 11 Feb 2014 18:53:30 +0100
Subject: [Buildroot] [RFC PATCH] toolchain-external: instrument wrapper
 to warn about unsafe paths
In-Reply-To: <20140211091855.00e6c483@skate>
References: <1392074881-12508-1-git-send-email-thomas.petazzoni@free-electrons.com>
 <20140211003326.GH5239@free.fr> <20140211091855.00e6c483@skate>
Message-ID: <20140211175330.GA3411@free.fr>
List-Id: <buildroot.busybox.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: buildroot@busybox.net

Thomas, All,

On 2014-02-11 09:18 +0100, Thomas Petazzoni spake thusly:
> On Tue, 11 Feb 2014 01:33:27 +0100, Yann E. MORIN wrote:
> 
> > > Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the
> > > external wrapper will instead error out and abort the compilation. We
> > > could then one day imagine setting this BR_PARANOID_WRAPPER in the
> > > autobuilders.
> > 
> > I think I'd prefer we do as for BR_DEBUG_WRAPPER:
> >     BR2_PARANOID_WRAPPER undefined or empty: nothing
> >     BR2_PARANOID_WRAPPER=WARNING           : warning
> >     BR2_PARANOID_WRAPPER=ERROR             : error out
> 
> Sure, I don't have any particular feeling about this. However, I would
> maybe like to have this feature enabled as the warning level by default.

Probably sane, indeed.

> > Side-note: BR_DEBUG_WRAPPER should probably be renamed to
> > BR2_DEBUG_WRAPPER, to follow the newly-stated naming scheme, no?
> 
> If I understood the newly-stated naming scheme, yes, I believe you're
> right, it should be renamed BR2_DEBUG_WRAPPER.

Ok, will cook a patch.

> > > A similar change could be made to the internal toolchain backend
> > > either by making it use a wrapper like the external toolchain one, or
> > > by adding some patches to gcc, by borrowing the changes made by the
> > > CodeSourcery people.
> > 
> > Maybe it would be best to not duplicate this, and always use the
> > wrapper, even for the internal backend?
> 
> Which is exactly one of the two solutions I proposed in the paragraph
> you're quoting :-)

And I was just expressing my preference. ;-)

> > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > > ---
> > >  .../toolchain-external/ext-toolchain-wrapper.c     | 27 ++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > > index 81c6ed1..c8dcad5 100644
> > > --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > > +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > > @@ -77,6 +77,7 @@ int main(int argc, char **argv)
> > >  	char *progpath = argv[0];
> > >  	char *basename;
> > >  	char *env_debug;
> > > +	char *paranoid_wrapper;
> > >  	int ret, i, count = 0, debug;
> > >  
> > >  	/* Calculate the relative paths */
> > > @@ -178,6 +179,32 @@ int main(int argc, char **argv)
> > >  	}
> > >  #endif /* ARCH || TUNE || CPU */
> > >  
> > > +	paranoid_wrapper = getenv("BR_PARANOID_WRAPPER");
> > > +
> > > +	/* Check for unsafe library and header paths */
> > > +	for (i = 1; i < argc; i++) {
> > > +		if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) ||
> > > +		    !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) {

Don't we also want to check -L/lib as well?

> > No need for strncmp: you're comparing to a constant, so it will not
> > overflow.
> 
> I need strncmp here: I want to match only the first characters of
> argv[i]. I.e, the above condition should match:
> 
> 	-I/usr/bar
> 	-L/usr/local/foo
> 	-I/usr/include/mysql
> 
> etc. If I use strcmp(), it is going to compare the entire string, and
> -I/usr/include/mysql is not the same as -I/usr.

Doh, righto-right. I should not review patches so late in the night...

> > Also, using strlen on an argument of strncmp is flawed to begin with.
> 
> Weird, a certain Yann E. Morin did exactly this in
> http://git.buildroot.net/buildroot/commit/toolchain/toolchain-external/ext-toolchain-wrapper.c?id=2c1dc32647eb308126b0ae80a91988059d39aa7b :-)

Hehe! ;-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'