[PATCH i-g-t v4] Fix memory access issue due to variable block scope

[Peter Senna Tschudin]

On Thu, Mar 28, 2024 at 7:55 AM Peter Senna Tschudin <me at petersenna.com> wrote:
>
> On Wed, Mar 27, 2024 at 7:43 PM Kamil Konieczny
> <kamil.konieczny at linux.intel.com> wrote:
> >
> > Hi Peter,
> > On 2024-03-27 at 07:59:27 +0100, Peter Senna Tschudin wrote:
> >
> > One nit about subject, as you can see on patchwork
> > https://patchwork.freedesktop.org/project/igt/series/?ordering=-last_updated
> >
> > we use test name as prefix for a patch, so instead of:
> >
> > [PATCH i-g-t v4] Fix memory access issue due to variable block
> >
> > it should be:
> >
> > [PATCH i-g-t v4] tests/intel/gem_exec_capture: Fix many-* subtests
>
> Oops, my bad. Thank you for pointing this out, I will fix it.
>
> >
> > > This patch fixes the tests many-4k-incremental and many-4k-zero from
> > > gem_exec_capture that are currently failing with an invalid file
> > > descriptor error.
> > >
> > > tests/intel/gem_exec_capture.c
> > >     many(), userptr(), capture_invisible()
> > >         find_first_available_engine()
> > >             for_each_ctx_engine()
> > >                 for_each_ctx_cfg_engine()
> > >
> > > find_first_available_engine() expects a struct intel_execution_engine2
> > > *e as parameter. for_each_ctx_cfg_engine() allocates a struct
> > > intel_engine_data ed inside a for loop and then update e to point to an
> > > element of ed.
> > >
> > > The problem is that ed has the block scope of the for loop, and trying
> > > to access its content through e after the for loop has ended creates
> > > undefined behavior.
> > >
> > > This patch updates e to point to the same content at a different memory
> > > region, to avoid the block scope of ed.
> > >
> > > v4: Move `saved = configure_hangs(fd, e, ctx->id);` to inside the for
> > > loop where it is safe to use 'e'. This is because 'e' points to an
> > > element of a variable that is defined with the block scope of the for
> > > loop.
> >
> > Yes, this way it works but I still think it would be better
> > to use function for it.
>
> I have been there and I failed to find what better means. The core of
> the problem is assigning a component from ed to e. In the macro it
> does not work because ed has the scope of the for loop. Using a
> function simply changes the scope of ed from the for loop to the
> function and the same problem happens. Solutions include:
>  - Adding `e = &saved.engine;` at the end of the function. This
> updates e to an address without scope limitations. (saved is returned
> by the function)
>  - Making ed static: Elegant but will break the code if there is ever
> concurrency
>  - Removing e from the function arguments. This is ok, but then after
> calling the function we will need to add `e = &saved.engine;`
>
> So as the best solution seems to be adding `e = &saved.engine;` at the
> end of the function, doing that at the existing macro felt like a
> better approach.
>
> >
> > >
> > > Signed-off-by: Peter Senna Tschudin <me at petersenna.com>
> > > ---
> > >  tests/intel/gem_exec_capture.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/intel/gem_exec_capture.c b/tests/intel/gem_exec_capture.c
> > > index 57b178f3e..f7b10bdd4 100644
> > > --- a/tests/intel/gem_exec_capture.c
> > > +++ b/tests/intel/gem_exec_capture.c
> > > @@ -665,10 +665,12 @@ static bool needs_recoverable_ctx(int fd)
> > >               ctx = intel_ctx_create_all_physical(fd); \
> > >               igt_assert(ctx); \
> > >               for_each_ctx_engine(fd, ctx, e) \
> > > -                     for_each_if(gem_class_can_store_dword(fd, e->class)) \
> > > +                     for_each_if(gem_class_can_store_dword(fd, e->class)) { \
> > > +                             saved = configure_hangs(fd, e, ctx->id); \
> >
> > Yes, it is a fix for a problem but something strange is
> > happening here, imho we need to debug it more.
> >
> > >                               break; \
> > >               } \
> > >               e = &saved.engine; \
> > >                       igt_assert(e); \
> >
> > Two above lines are not needed, 'e' will always be != NULL.
>
> The  `igt_assert(e);` was already there and I kept it. The other line
> is the actual fix for the problem. `e = &saved.engine;` updates e to
> point to the same content but to an address that has no scope
> limitation. There is no scope restrictions because saved is returned
> by the function.

Oops, there is no scope restrictions because 'saved' and 'e' are both
defined in many(), the function that calls
find_first_available_engine(). The change I am proposing is safe
because 'saved' and 'e' have the same variable scope: the function
many(). Before my change, 'e' had the broad scope of many() while it
was pointing to 'ed' which had the very limited scope of the
for_each_ctx_engine() macro.