GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-33330: PyImport_Cleanup check for exc leaks by vstinner · Pull Request #7068 · python/cpython

Conversation

vstinner commented May 23, 2018 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Add "assert(!PyErr_Occurred());" assertions to PyImport_Cleanup()
and _PyGC_CollectNoFail() to make sure that these functions don't
leak exceptions.
Replace also PyImport_GetModuleDict() call with interp->modules to
prevent a fatal error if modules is already NULL.

https://bugs.python.org/issue33330

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          bpo-33330: PyImport_Cleanup check for exc leaks

52b8028

* Add "assert(!PyErr_Occurred());" assertions to PyImport_Cleanup()
  and _PyGC_CollectNoFail() to make sure that these functions don't
  leak exceptions.
* Replace also PyImport_GetModuleDict() call with interp->modules to
  prevent a fatal error if modules is already NULL.

vstinner requested review from a team, ericsnowcurrently and serhiy-storchaka

May 23, 2018 09:15

the-knights-who-say-ni added the CLA signed label

May 23, 2018

bedevere-bot added the awaiting merge label

May 23, 2018

vstinner commented May 23, 2018

Copy link

Copy Markdown

Member Author

The commit d393c1b replaced interp->modules with PyImport_GetModuleDict(), but this function fails with a fatal error if modules is NULL. I'm not sure that it was a deliberate change. Moreover, this commit has been partially reverted (interp->modules is back!).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner added the skip news label

May 23, 2018

vstinner commented May 23, 2018

Copy link

Copy Markdown

Member Author

PyImport_Cleanup() is part of the public API, but it's documented as "Empty the module table. For internal use only." :-( Why do we have public API for internal use only? Likely a mistake from the past.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

May 23, 2018

View reviewed changes

Comment thread

Python/import.c

		void
		PyImport_Cleanup(void)
		{
		assert(!PyErr_Occurred());

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it is worth to call PyErr_WriteUnraisable() here. This will expose some information about the exception raised outside of this function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner May 23, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contract of this function is that the caller must not hold an exception. If the assertion fails, I would prefer to fix the caller rather than working around the bug using PyErr_WriteUnraisable().

The point is not only to catch bugs in the current code, but also ease debug when the callers code will be modified. Python shutdown process is very fragile and error prone.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/gcmodule.c

		during interpreter shutdown (and then never finish it).
		See http://bugs.python.org/issue8713#msg195178 for an example.
		*/
		assert(!PyErr_Occurred());

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesterday I wrote a patch that improves errors handling in the garbage collector. bpo-33612 is the result of testing it. And I have added similar asserts in _PyGC_CollectNoFail() (but the patch is not completed yet, maybe I'll move them).

At least move the second assert inside the "else" block.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c

		if (PyDict_CheckExact(modules)) {
		pos = 0;
		while (PyDict_Next(modules, &pos, &key, &value)) {
		assert(!PyErr_Occurred());

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither PyDict_Next() nor any other calls before can raise an exception.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner May 23, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the theory. In practice, something called before might leak an exception. It's to catch bugs "just in case". The assertion is cheap. Debugging an "exception leak" can be very painful.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no code between this line and previous error check that can raise an exception and don't silence it. I have checked this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c

		PyErr_WriteUnraisable(NULL);
		continue;
		}
		assert(!PyErr_Occurred());

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyObject_GetIter(), PyIter_Next() and PyObject_GetItem() can raise and exception if corresponding slots are implemented incorrectly in extensions. assert looks good here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c

		Py_DECREF(iterator);
		}
		}
		assert(!PyErr_Occurred());

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never fails.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c

		PyErr_Clear();
		}
		Py_XDECREF(dict);
		assert(!PyErr_Occurred());

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never fails.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c

		references left to it), we need to delete the "builtins"
		module last. Likewise, we don't delete sys until the very
		end because it is implicitly referenced (e.g. by print). */
		assert(!PyErr_Occurred());

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never fails.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/import.c

		Py_INCREF(mod);
		if (Py_VerboseFlag && PyUnicode_Check(name))
		PySys_FormatStderr("# cleanup[3] wiping %U\n", name);
		assert(!PyErr_Occurred());

serhiy-storchaka May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never fails.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka commented May 23, 2018

Copy link

Copy Markdown

Member

I would left only few asserts after calls that can raise exceptions if the third-party code has bugs. And one at the end of PyImport_Cleanup() just for sure. For other asserts we can guarantee the absence of exceptions.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented May 23, 2018

Copy link

Copy Markdown

Member Author

The last 2 years, I added many "assert(!PyErr_Occurred());" in ceval.c and object.c, and it helped me a lot to detect some very old bugs. The rationale here is the same.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ericsnowcurrently reviewed

May 23, 2018

View reviewed changes

Comment thread

Python/import.c

		const char * const *p;

		if (modules == NULL)
		PyObject *modules = interp->modules;

ericsnowcurrently May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather avoid this, if possible. Under what conditions does the use of PyImport_GetModuleDict() cause a fatal error?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner May 23, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you call PyImport_Cleanup() twice. In Python 3.6, calling PyImport_Cleanup() a second time does nothing: that's the purpose of the test on the next line.

PyImport_Cleanup() is part of the public API, even if it's documented as "please don't call this function" ;-) I would prefer to restore Python 3.6 behaviour.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reacted with thumbs up emoji

All reactions

👍 1 reaction

ericsnowcurrently May 23, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for clarifying. LGTM then. I still plan on landing #3606 at some point, but at that point I'll have to deal with recent interp->modules usage anyway. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka commented May 26, 2018

Copy link

Copy Markdown

Member

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

May 28, 2018

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except that GC is covered by a separate PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka approved these changes

May 28, 2018

View reviewed changes

vstinner commented May 28, 2018

Copy link

Copy Markdown

Member Author

LGTM, except that GC is covered by a separate PR.

I will wait until #7126 is merged, and then rebase this PR on top of it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner closed this

Jul 16, 2018

vstinner deleted the pyimport_cleanup_assert branch

July 16, 2018 14:51

serhiy-storchaka commented Jul 16, 2018

Copy link

Copy Markdown

Member

Why not rebase and merge this PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Jul 16, 2018

Copy link

Copy Markdown

Member Author

It's just that I wrote it 2 months ago, and I lost interest in this minor enhancement in the meanwhile. I'm trying to reduce my number of open PRs. Feel free to reuse my patch to create a new PR if you want.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka mentioned this pull request

Jun 20, 2022

Better error handling in PyImport_Cleanup() #77511

Closed

Reviewers

ericsnowcurrently

ericsnowcurrently left review comments

serhiy-storchaka

serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

vstinner commented May 23, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented May 23, 2018

Uh oh!

vstinner commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented May 23, 2018

Uh oh!

vstinner commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented May 26, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 28, 2018

Uh oh!

serhiy-storchaka commented Jul 16, 2018

Uh oh!

vstinner commented Jul 16, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vstinner commented May 23, 2018 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.