The new_interpreter() function incorrectly returns _PyStatus_OK() on some error code paths

[colesbury]

Bug report

The following code paths should probably returns some sort of exception status (possibly _PyStatus_NO_MEMORY()).

cpython/Python/pylifecycle.c

Lines 2084 to 2088 in a1551b4

	PyInterpreterState *interp = PyInterpreterState_New();
	if (interp == NULL) {
	*tstate_p = NULL;
	return _PyStatus_OK();
	}

cpython/Python/pylifecycle.c

Lines 2090 to 2096 in a1551b4

	PyThreadState *tstate = _PyThreadState_New(interp,
	_PyThreadState_WHENCE_INTERP);
	if (tstate == NULL) {
	PyInterpreterState_Delete(interp);
	*tstate_p = NULL;
	return _PyStatus_OK();
	}

Note that PyInterpreterState_New() currently never returns NULL -- it calls Py_FatalError() instead -- but we should probably still handle that case as well.

_PyThreadState_New can return NULL when out of memory.

cc @ericsnowcurrently

[ericsnowcurrently]

@vstinner, looks like you made the relevant change 6 years ago in gh-4412. Do you have an objections to returning an error status in those two cases?

[kumaraditya303]

The proposed changes SGTM

[vstinner]

Py_NewInterpreter() returns NULL if PyInterpreterState_New() or _PyThreadState_New() returns NULL. Sadly, this behavior is not documented: https://docs.python.org/dev/c-api/init.html#c.Py_NewInterpreter

It's possible to recover from Py_NewInterpreter() returning NULL, whereas calling Py_ExitStatusException() kills the process: it's not possible to recover from such error.

Note that PyInterpreterState_New() currently never returns NULL -- it calls Py_FatalError() instead -- but we should probably still handle that case as well.

Are you sure about that?

[colesbury]

@vstinner, please take a look at the current implementations. You can also test it yourself by injecting a return NULL in _PyThreadState_New()

PyInterpreterState_New() calls Py_ExitStatusException on error. It doesn't return NULL.

cpython/Python/pystate.c

Lines 756 to 769 in 2d9d3a9

	PyInterpreterState *
	PyInterpreterState_New(void)
	{
	// tstate can be NULL
	PyThreadState *tstate = current_fast_get();
	PyInterpreterState *interp;
	PyStatus status = _PyInterpreterState_New(tstate, &interp);
	if (_PyStatus_EXCEPTION(status)) {
	Py_ExitStatusException(status);
	}
	assert(interp != NULL);
	return interp;
	}

You can read through the called _PyInterpreterState_New(). It only sets *pinterp to NULL if it also returns an exception status, which means that PyInterpreterState_New() never returns NULL.

Py_NewInterpreter() also never returns NULL. If _PyThreadState_New() fails, new_interpreter returns an exception status, which leads to Py_ExitStatusException(). And PyInterpreterState_New() doesn't return NULL (see above).

[vstinner]

I looked at the code and it looks dangerous to change _PyInterpreterState_New() and PyInterpreterState_New() to return NULL on memory allocation error, simply because it wasn't possible before. Memory allocation failure is unlikely and so I'm ok with using _PyStatus_NO_MEMORY() in this case.