Re: [Xen-devel] [PATCH] xen/arm: fix build after 2e35cdf

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

Title: You should at least mention this is for op-tee.

Also, mostly likely the sha1 is too small and likely to match multiple 
commit in the future. So you want to specify the title of the commit.
On 6/19/19 10:24 PM, Stefano Stabellini wrote:
> Optee breaks the build with:
>
> optee.c: In function ‘translate_noncontig.isra.4’:
> optee.c:743:38: error: ‘xen_data’ may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
>               xen_data->next_page_data = page_to_maddr(xen_pgs + 1);
>                                        ^
> optee.c:732:71: error: ‘guest_data’ may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
>           page = get_domain_ram_page(gaddr_to_gfn(guest_data->pages_list[idx]));
>                                                                         ^
> optee.c:750:21: error: ‘guest_pg’ may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
>               put_page(guest_pg);
>                       ^
> cc1: all warnings being treated as errors
>
> Fix it by initializing xen_data, guest_data, guest_pg to NULL. Also set
> xen_pgs to NULL for consistency.
Without more explanation I think this is an unwise choice. If GCC thinks 
it is going to be used unitialized, then mostly likely you silent an 
error that could end up to dereference NULL.
Also, setting xen_pgs for consistency will only defeat the compiler. 
Leading to dereferencing NULL and crash Xen...
For xen_pgs, this should definitely not be NULL. For the two others, you 
need to explain why this is fine (if this is just because the compiler 
can't find the reason, then you should add a comment in the code to 
explain it).
> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
>
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 28d34360fc..4825cc5410 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -663,7 +663,7 @@ static int translate_noncontig(struct optee_domain *ctx,
>       unsigned int order;
>       unsigned int idx = 0;
>       gfn_t gfn;
> -    struct page_info *guest_pg, *xen_pgs;
> +    struct page_info *guest_pg = NULL, *xen_pgs = NULL;
>       struct optee_shm_buf *optee_shm_buf;
>       /*
>        * This is memory layout for page list. Basically list consists of 4k 
> pages,
> @@ -675,7 +675,7 @@ static int translate_noncontig(struct optee_domain *ctx,
>       struct {
>           uint64_t pages_list[PAGELIST_ENTRIES_PER_PAGE];
>           uint64_t next_page_data;
> -    } *guest_data, *xen_data;
> +    } *guest_data = NULL, *xen_data = NULL;
>   
>       /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */
>       offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel