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

[Petri Latvala]

On Thu, Mar 21, 2019 at 12:42:13PM +0200, Petri Latvala wrote:
> 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).


And merged.


-- 
Petri Latvala