No check NULL after call malloc function

[xmat111998]

Description

While reviewing the zend_register_functions function in php/Zend/zend_API.c, I noticed several lines that call malloc without checking for a NULL return value. This oversight could potentially lead to a crash in cases where memory allocation fails.

ZEND_API zend_result zend_register_functions(zend_class_entry *scope, const zend_function_entry *functions, HashTable *function_table, int type) /* {{{ */
{
...

			/* Treat return type as an extra argument */
			num_args++;
			new_arg_info = malloc(sizeof(zend_arg_info) * num_args);
			memcpy(new_arg_info, arg_info, sizeof(zend_arg_info) * num_args);
			reg_function->common.arg_info = new_arg_info + 1;
			for (i = 0; i < num_args; i++) {
				if (ZEND_TYPE_HAS_CLASS(new_arg_info[i].type)) {
					ZEND_ASSERT(ZEND_TYPE_HAS_NAME(new_arg_info[i].type)
						&& "Should be stored as simple name");
					const char *class_name = ZEND_TYPE_LITERAL_NAME(new_arg_info[i].type);

					size_t num_types = 1;
					const char *p = class_name;
					while ((p = strchr(p, '|'))) {
						num_types++;
						p++;
					}

					if (num_types == 1) {
						/* Simple class type */
						ZEND_TYPE_SET_PTR(new_arg_info[i].type,
							zend_string_init_interned(class_name, strlen(class_name), 1));
					} else {
						/* Union type */
						zend_type_list *list = malloc(ZEND_TYPE_LIST_SIZE(num_types));
						list->num_types = num_types;
						ZEND_TYPE_SET_LIST(new_arg_info[i].type, list);

...
	}
	return SUCCESS;
}

Specifically, on the line:

new_arg_info = malloc(sizeof(zend_arg_info) * num_args);

Can we add some line to check the return NULL of them?

PHP Version

PHP 8.0.28

Operating System

Linux

[nielsdos]

These are only called at start up, if these fail there's nothing we can do about it anyway.

[xmat111998]

These are only called at start up, if these fail there's nothing we can do about it anyway.

So, do you mean that if something fails or crashes due to this function, we should just restart it and there's nothing else we can do?

[iluuu1994]

@nielsdos I think what @xmat111998 is referencing is that we should use pemalloc(sizeof(zend_arg_info) * num_args, true) (which will abort if unsuccessful), rather than assuming malloc was successful. These are still direct mallocs on master.

[cmb69]

I agree that it's better to abort() than to trigger an inevitable segfault (or worse).

[nielsdos]

Right, my point was that the end result is the same: we crash, during startup. So there is little difference actually between segfault and abort. If we already are OOM during startup then there's no hope at all.

[iluuu1994]

Ok, I looked through Zend/ and apparently there are indeed many other cases like this.

Commit 5a482a1:

/home/ilutov/Developer/php-src/Zend/zend_alloc.c
  3032,52: 		zend_mm_heap *mm_heap = alloc_globals->mm_heap = malloc(sizeof(zend_mm_heap));
  3048,30: 			mm_heap->tracked_allocs = malloc(sizeof(HashTable));

/home/ilutov/Developer/php-src/Zend/zend_API.c
  3056,18: 		reg_function = malloc(sizeof(zend_internal_function));
  3123,19: 			new_arg_info = malloc(sizeof(zend_internal_arg_info) * num_args);
  3149,30: 						zend_type_list *list = malloc(ZEND_TYPE_LIST_SIZE(num_types));
  3464,34: 	zend_class_entry *class_entry = malloc(sizeof(zend_class_entry));

/home/ilutov/Developer/php-src/Zend/zend_call_stack.c
  636,9: 	*ptr = malloc(len);

/home/ilutov/Developer/php-src/Zend/zend_constants.c
  113,37: 	EG(zend_constants) = (HashTable *) malloc(sizeof(HashTable));

/home/ilutov/Developer/php-src/Zend/zend_ini.c
  101,49: 	registered_zend_ini_directives = (HashTable *) malloc(sizeof(HashTable));
  169,37: 	EG(ini_directives) = (HashTable *) malloc(sizeof(HashTable));

/home/ilutov/Developer/php-src/Zend/zend_list.c
  270,8: 	lde = malloc(sizeof(zend_rsrc_list_dtors_entry));

/home/ilutov/Developer/php-src/Zend/zend_types.h
  1125,18: 		(zend_array *) malloc(sizeof(zend_array));				\
  1166,21: 		(zend_resource *) malloc(sizeof(zend_resource));		\
  1220,22: 		(zend_reference *) malloc(sizeof(zend_reference));		\

/home/ilutov/Developer/php-src/Zend/zend_vm_execute.skl
  99,24: 	zend_handlers_table = malloc(sizeof(HashTable));

/home/ilutov/Developer/php-src/Zend/zend.c
  717,51: 	compiler_globals->function_table = (HashTable *) malloc(sizeof(HashTable));
  722,48: 	compiler_globals->class_table = (HashTable *) malloc(sizeof(HashTable));
  728,49: 	compiler_globals->auto_globals = (HashTable *) malloc(sizeof(HashTable));
  1006,40: 	GLOBAL_FUNCTION_TABLE = (HashTable *) malloc(sizeof(HashTable));
  1007,37: 	GLOBAL_CLASS_TABLE = (HashTable *) malloc(sizeof(HashTable));
  1008,44: 	GLOBAL_AUTO_GLOBALS_TABLE = (HashTable *) malloc(sizeof(HashTable));
  1009,41: 	GLOBAL_CONSTANTS_TABLE = (HashTable *) malloc(sizeof(HashTable));
  1029,51: 	compiler_globals->function_table = (HashTable *) malloc(sizeof(HashTable));
  1030,48: 	compiler_globals->class_table = (HashTable *) malloc(sizeof(HashTable));
  1295,22: 	new_info = (char *) malloc(new_info_length + 1);

If we decide to do anything here, it only makes sense to adjust all cases, there are likely more in extensions. I wouldn't object to doing so, given that persistent allocations are rare and thus not performance critical. But given there are no reports, maybe it's not something that needs fixing.

[nielsdos]

It's something static analysers typically trip over, and I suppose this bug report was also made by SA

[iluuu1994]

Right. If it's a common false positive, it makes sense to fix it. But I would also imagine there are lots of false positive for many static analyzers in any case. You can probably answer this better than me. 🙂

[nielsdos]

It's one of the classic things analysers look for, many production ones and many research papers find all sorts of different ways to detect these. So it may be worth it to handle these codebase-wide if more reports come in.