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

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.