Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-43951: Two minor fixes for accessing a module's name. #25658

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

larryhastings
Copy link
Contributor

@larryhastings larryhastings commented Apr 27, 2021

While working on another issue, I noticed two minor nits in the C implementation of the module object. Both are related to getting a module's name.

First, the C function module_dir() (aka module.__dir__) starts by ensuring the module dict is valid. If the module dict is invalid, it wants to format an exception using the name of the module, which it gets from PyModule_GetName(). However, PyModule_GetName() gets the name of the module from the dict. So getting the name in this circumstance will never succeed.

When module_dir() wants to format the error but can't get the name, it knows that PyModule_GetName() must have already raised an exception. So it leaves that exception alone and returns an error. The end result is that the exception raised here is kind of useless and misleading: dir(module) on a module with no __dict__ raises SystemError("nameless module"). I changed the code to actually raise the exception it wanted to raise, just without a real module name: TypeError("<module>.__dict__ is not a dictionary"). This seems more useful, and would do a better job putting the programmer who encountered this on the right track of figuring out what was going on.

Second, the C API function PyModule_GetNameObject() checks to see if the module has a dict. If m->md_dict is not NULL, it calls _PyDict_GetItemIdWithError(). However, it's possible for m->md_dict to be None. And if you call _PyDict_GetItemIdWithError(Py_None, ...) it will crash.

Unfortunately, this crash was due to my own bug in the other branch. Fixing my code made the crash go away. I assert that this is still possible at the API level.

The fix is easy: add a PyDict_Check() to PyModule_GetNameObject().

Unfortunately, I don't know how to add a unit test for this. Having changed module_dir() above, I can't find any other interfaces callable from Python that eventually call PyModule_GetNameObject(). So I don't know how to trick the runtime into reproducing this error.

https://bugs.python.org/issue43951

PyErr_Format(PyExc_TypeError,
"%.200s.__dict__ is not a dictionary",
name);
PyErr_Format(PyExc_TypeError, "<module>.__dict__ is not a dictionary");
Copy link
Member

Choose a reason for hiding this comment

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

Why not using %R to use repr(module)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think of it. Would that produce a better result, if we had one of these broken, nameless, dict-less module objects?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@bedevere-bot
Copy link

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

@larryhastings larryhastings deleted the fix_module_bugs branch April 30, 2021 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants