[Mesa-dev] [PATCH 2/3] mesa/main: avoid null access in format_array_table_init()

[Jose Fonseca]

On 05/05/15 11:50, Juha-Pekka Heikkila wrote:
> If _mesa_hash_table_create failed we'd get null pointer. Report
> error and go away.
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> ---
>   src/mesa/main/formats.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
> index 8af44e9..f7c9402 100644
> --- a/src/mesa/main/formats.c
> +++ b/src/mesa/main/formats.c
> @@ -397,6 +397,11 @@ format_array_format_table_init(void)
>      format_array_format_table = _mesa_hash_table_create(NULL, NULL,
>                                                          array_formats_equal);
>
> +   if (!format_array_format_table) {
> +      _mesa_error_no_memory(__func__);
> +      return;
> +   }
> +
>      for (f = 1; f < MESA_FORMAT_COUNT; ++f) {
>         info = _mesa_get_format_info(f);
>         if (!info->ArrayFormat)
> @@ -432,6 +437,11 @@ _mesa_format_from_array_format(uint32_t array_format)
>
>      call_once(&format_array_format_table_exists, format_array_format_table_init);
>
> +   if (!format_array_format_table) {
> +      format_array_format_table_exists = ONCE_FLAG_INIT;

This is not portable, as ONCE_FLAG_INIT is meant to be an initializer 
expression.  In particular it's defined as a structure initializer on 
Windows "{ 0 }" and is not a valid rvalue expression.

I've just fixed the build, but this still looks like a bad idea:

-  the idea of call_once is "calling once", not "keep trying" -- and 
this usage can easily lead to leaks crashes, etc, depending on how 
call_once was implemented.

- touching touching format_array_format_table_exists introduces a race 
condition: imagine another call_once happens when 
format_array_format_table_exists is being overwritten, and catches half 
written


I also don't understand what's exactly the problem here.  Why would 
_mesa_hash_table_create() fail?  I think it might be better to just 
abort(), rather than this sort of half-remedies.

Maybe we need a `_mesa_error_no_memory_fatal` or add a `fatal` parameter 
to `_mesa_error_no_memory`.

Jose