Re: [igt-dev] [PATCH i-g-t v3] tests/testdisplay: fix heap overflow

[Petri Latvala <petri.latvala@intel.com>]

On Thu, Mar 21, 2019 at 12:32:24PM +0200, Ser, Simon wrote:
> On Wed, 2019-03-20 at 14:17 +0200, Petri Latvala wrote:
> > On Wed, Mar 20, 2019 at 12:00:13PM +0000, Chris Wilson wrote:
> > > Quoting Simon Ser (2019-03-20 11:48:57)
> > > > +       argv0 = strdup(argv[0]);
> > > > +       igt_assert(argv0);
> > > > +       exec_path = dirname(argv0);
> > > >         ret = chdir(exec_path);
> > > >         igt_assert_eq(ret, 0);
> > > 
> > > One should ask Petri if igt_assert_eq() is even legal inside the
> > > helper
> > > (i.e. outside of igt_main and igt_subtest).
> > 
> > *opens testdisplay.c*
> > 
> > *finds main()*
> > 
> > *runs away screaming*
> > 
> > 
> > Short answer: It's not legal there
> > 
> > Long answer:
> > It would be legal there if appropriate steps are taken to ensure IGT
> > core knows it's a test without subtests. testdisplay, being a
> > dinosaur
> > that hasn't realized it's pushing up the daisies, doesn't use
> > igt_simple_main, or call igt_simple_init_parse_opts, or otherwise do
> > the common things any recently written test is doing.
> > 
> > It's also calling igt_skip_on_simulation in just about the only
> > possible context where it's not legal.
> > 
> > Note to self: Hurry up with removing all custom main functions.
> 
> Hmm. So would you prefer to add an error return value to this function,
> or to just continue to use these even if they don't work and fix
> everything in a later commit?


No no, the patch is good. All that is wrong with the code has already
been wrong, and will be fixed by replacing its custom main() with
igt_simple_main_custom_argv_handling (implementation pending).


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev