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

[Andi Shyti <andi.shyti@linux.intel.com>]

Hey Peter,

On Mon, Mar 25, 2024 at 10:35:48PM +0100, Peter Senna Tschudin wrote:
> This patch fixes the tests gem_exec_capture@many-4k-incremental and
> gem_exec_capture@many-4k-zero that are currently failing with an invalid file
> descriptor error.

where is gem_exec_capture calling for_each_ctx_cfg_engine()?

>  struct intel_execution_engine2 *
>  intel_get_current_engine(struct intel_engine_data *ed)
> 
> When intel_get_current_engine is called from the macro
> for_each_ctx_cfg_engine(), the variable *ed is defined within a for loop. The
> scope of *ed is limited to that loop, leading to access violations when
> attempting to access its contents outside the loop.
> 
> Before to this patch, intel_get_current_engine() would return an element of *ed
> and attempting to use it after the loop ended resulted in undefined behavior.
> 
> This patch introduces a memcpy() to copy the contents of ed->current_engine to
> a memory area not confined by the loop's scope, ensuring safe access to the
> data.
> 
> v2: Added 'i-g-t' to the Subject.
> 
> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
> ---
>  lib/i915/gem_engine_topology.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index afb576afb..b3b809482 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -189,12 +189,24 @@ static int __query_engine_list(int fd, struct intel_engine_data *ed)
>  struct intel_execution_engine2 *
>  intel_get_current_engine(struct intel_engine_data *ed)
>  {
> +	struct intel_execution_engine2 *ret = NULL;
> +
>  	if (ed->n >= ed->nengines)
>  		ed->current_engine = NULL;
>  	else if (!ed->n)
>  		ed->current_engine = &ed->engines[0];
>  
> -	return ed->current_engine;
> +	// When called from the macro for_each_ctx_cfg_engine(), *ed is defined
> +	// inside a for loop. In that case, not memcping ed->current_engine
> +	// will lead to a memory access violation when trying to access the
> +	// contents of ed->current_engine after the end of the for loop

can you please use /* ... */ style of comment?

> +	if (ed->current_engine) {
> +		ret = malloc(sizeof(*ret));

should this be freed at some point?

Andi