Re: [Tarantool-patches] [PATCH v20 1/7] box/schema: make sure hashes are created

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

05.04.2021 12:50, Cyrill Gorcunov пишет:
> On Mon, Apr 05, 2021 at 12:28:50PM +0300, Serge Petrenko wrote:
>>> diff --git a/src/box/schema.cc b/src/box/schema.cc
>>> index 963278b19..89904e4d2 100644
>>> --- a/src/box/schema.cc
>>> +++ b/src/box/schema.cc
>>> @@ -372,6 +372,13 @@ schema_init(void)
>>>    	funcs = mh_i32ptr_new();
>>>    	funcs_by_name = mh_strnptr_new();
>>>    	sequences = mh_i32ptr_new();
>>> +
>>> +	if (spaces == NULL || spaces_by_name == NULL ||
>>> +	    funcs == NULL || funcs_by_name == NULL ||
>>> +	    sequences == NULL) {
>>> +		panic("Can't allocate schema hashes");
>>> +	}
>> I've just noticed, all the mh_..._new methods will fail on a null
>> dereference even before returning. So the check above doesn't
>> make much sense.
> This is because our lib/salad/mhash.h is simply buggy and it must be:
>
> 1) fixed in mhash level
> 2) get rid of this macros madness and convert it to normal
>     library. this macros spreading is over the top already
>     and I don't understand why nobody cares about icache footprint
>
> Nevertheless we should check for x_new resuls as we do in our
> other code. Let walk over spaces
>
> schema_init
>    spaces = mh_i32ptr_new();
>    ...
>    sc_space_new
>      space_cache_replace(NULL, space);
>        mh_int_t k = mh_i32ptr_put(spaces, &node_p, ...
>
> which is simply nil dereferencing (once we fix bullet 1).
>
>> But I see that some other places have this check
>> `if (smth = mh_smth_new() == NULL)` and set an error.
>>
>> Am I missing something?
> I think the idea was that mh_x_new doesn't do nil dereference
> by its own but behaves in a safe way as any library routine
> should. That said I still think we should check for mh_i32ptr_new
> results. But won't insist to include this patch, we can drop it
> if you prefer.

I see, let's keep the patch then. LGTM.

-- 
Serge Petrenko