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

[Peter Senna Tschudin]

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.

[...]