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

[Kamil Konieczny]

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

> 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.

> 
> 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.

Regards,
Kamil

> -		saved = configure_hangs(fd, e, ctx->id); \
>  	} while(0)
>  
>  static void many(int fd, int dir, uint64_t size, unsigned int flags)
> -- 
> 2.44.0
>