[Mesa-dev] [PATCH 2/5] radeonsi: fix error paths of si_texture_transfer_map

[Timothy Arceri]

On 11/04/18 20:56, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> trans is zero-initialized, but trans->resource is setup immediately so
> needs to be dereferenced.
> ---
>   src/gallium/drivers/radeonsi/si_texture.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/src/gallium/drivers/radeonsi/si_texture.c b/src/gallium/drivers/radeonsi/si_texture.c
> index 0bab2a6c45b..907e8c8cec9 100644
> --- a/src/gallium/drivers/radeonsi/si_texture.c
> +++ b/src/gallium/drivers/radeonsi/si_texture.c
> @@ -1728,49 +1728,46 @@ static void *si_texture_transfer_map(struct pipe_context *ctx,
>   			 * then decompress the temporary one to staging.
>   			 *
>   			 * Only the region being mapped is transfered.
>   			 */
>   			struct pipe_resource resource;
>   
>   			si_init_temp_resource_from_box(&resource, texture, box, level, 0);
>   
>   			if (!si_init_flushed_depth_texture(ctx, &resource, &staging_depth)) {
>   				PRINT_ERR("failed to create temporary texture to hold untiled copy\n");
> -				FREE(trans);
> -				return NULL;
> +				goto fail_trans;
>   			}
>   
>   			if (usage & PIPE_TRANSFER_READ) {
>   				struct pipe_resource *temp = ctx->screen->resource_create(ctx->screen, &resource);
>   				if (!temp) {
>   					PRINT_ERR("failed to create a temporary depth texture\n");
> -					FREE(trans);
> -					return NULL;
> +					goto fail_trans;
>   				}
>   
>   				si_copy_region_with_blit(ctx, temp, 0, 0, 0, 0, texture, level, box);
>   				si_blit_decompress_depth(ctx, (struct r600_texture*)temp, staging_depth,
>   							 0, 0, 0, box->depth, 0, 0);
>   				pipe_resource_reference(&temp, NULL);
>   			}
>   
>   			/* Just get the strides. */
>   			si_texture_get_offset(sctx->screen, staging_depth, level, NULL,
>   						&trans->b.b.stride,
>   						&trans->b.b.layer_stride);
>   		} else {
>   			/* XXX: only readback the rectangle which is being mapped? */
>   			/* XXX: when discard is true, no need to read back from depth texture */
>   			if (!si_init_flushed_depth_texture(ctx, texture, &staging_depth)) {
>   				PRINT_ERR("failed to create temporary texture to hold untiled copy\n");
> -				FREE(trans);
> -				return NULL;
> +				goto fail_trans;
>   			}
>   
>   			si_blit_decompress_depth(ctx, rtex, staging_depth,
>   						 level, level,
>   						 box->z, box->z + box->depth - 1,
>   						 0, 0);
>   
>   			offset = si_texture_get_offset(sctx->screen, staging_depth,
>   							 level, box,
>   							 &trans->b.b.stride,
> @@ -1785,22 +1782,21 @@ static void *si_texture_transfer_map(struct pipe_context *ctx,
>   
>   		si_init_temp_resource_from_box(&resource, texture, box, level,
>   						 SI_RESOURCE_FLAG_TRANSFER);
>   		resource.usage = (usage & PIPE_TRANSFER_READ) ?
>   			PIPE_USAGE_STAGING : PIPE_USAGE_STREAM;
>   
>   		/* Create the temporary texture. */
>   		staging = (struct r600_texture*)ctx->screen->resource_create(ctx->screen, &resource);
>   		if (!staging) {
>   			PRINT_ERR("failed to create temporary texture to hold untiled copy\n");
> -			FREE(trans);
> -			return NULL;
> +			goto fail_trans;
>   		}
>   		trans->staging = &staging->resource;
>   
>   		/* Just get the strides. */
>   		si_texture_get_offset(sctx->screen, staging, 0, NULL,
>   					&trans->b.b.stride,
>   					&trans->b.b.layer_stride);
>   
>   		if (usage & PIPE_TRANSFER_READ)
>   			si_copy_to_staging_texture(ctx, trans);
> @@ -1809,28 +1805,31 @@ static void *si_texture_transfer_map(struct pipe_context *ctx,
>   
>   		buf = trans->staging;
>   	} else {
>   		/* the resource is mapped directly */
>   		offset = si_texture_get_offset(sctx->screen, rtex, level, box,
>   						 &trans->b.b.stride,
>   						 &trans->b.b.layer_stride);
>   		buf = &rtex->resource;
>   	}
>   
> -	if (!(map = si_buffer_map_sync_with_rings(sctx, buf, usage))) {
> -		r600_resource_reference(&trans->staging, NULL);
> -		FREE(trans);
> -		return NULL;
> -	}
> +	if (!(map = si_buffer_map_sync_with_rings(sctx, buf, usage)))
> +		goto fail_trans;
>   
>   	*ptransfer = &trans->b.b;
>   	return map + offset;
> +
> +fail_trans:
> +	r600_resource_reference(&trans->staging, NULL);

this needs to be:

if (trans->staging)
    r600_resource_reference(&trans->staging, NULL);

Otherwise:

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>

> +	pipe_resource_reference(&trans->b.b.resource, NULL);
> +	FREE(trans);
> +	return NULL;
>   }
>   
>   static void si_texture_transfer_unmap(struct pipe_context *ctx,
>   				      struct pipe_transfer* transfer)
>   {
>   	struct si_context *sctx = (struct si_context*)ctx;
>   	struct r600_transfer *rtransfer = (struct r600_transfer*)transfer;
>   	struct pipe_resource *texture = transfer->resource;
>   	struct r600_texture *rtex = (struct r600_texture*)texture;
>   
>