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

Fixed crashed in _dill._is_builtin_module when a module's __file__ is None #598

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

miguelinux314
Copy link
Contributor

Summary

The _is_builtin_module function of dill/_dill.py has the following check in line 1587

if not hasattr(module, "__file__"): return True

If the check is not passed, then os.path.realpath(module.__file__) is called later in that function (line 1591).

This causes dill to crash when a module has __file__ set to None. This PR fixes that check so that when module.__file__ is None, the module is recognized as not builtin.

Notes:

  • Even though https://docs.python.org/3/reference/import.html#file__ specifies that when __file__ is set it is a string, it will actually be none when importing a module without an __init__.py file in it.
  • This PR only changes the behavior of dill to prevent TypeError crashes. Code that did not crash before is unaffected by this PR.
  • No issue created prior to this PR)

@mmckerns
Copy link
Member

mmckerns commented Jun 6, 2023

@miguelinux314: Thanks for catching this. In theory, __file__ could be set to anything, right? So, to mitigate a TypeError, would it be better to either type-check (i.e. with type) or to catch a TypeError? If None is the most common case, then you could check that as you have now first...

@mmckerns mmckerns added the bugfix label Jun 6, 2023
@mmckerns mmckerns added this to the dill-0.3.7 milestone Jun 6, 2023
@miguelinux314
Copy link
Contributor Author

Thank you @mmckerns for the quick reply.

Yes, the __file__ attribute of any module (built-in or not) can be manually set to anything.

On the other hand, under certain conditions (see https://github.com/python/cpython/blob/423459be2f0b6d007e5f235f39d80044cb099faf/Lib/importlib/_bootstrap.py#L667, caused by https://bugs.python.org/issue32305) importlib does set it to None without complaining or manually fidgeting with the __file__ attribute.

Based on this, I don't think one can get the __file__ attribute to differ both from a valid path string and None without actively trying to break the code. That's why I checked only for None and left other cases to crash. However, if you feel the dill library would be sturdier considering the manual fiddling cases, my recommendation would be a try-catch of lines 1620 to 1629 with TypeError. I'd be happy with either solution.

Thanks again for you time and help

@mmckerns mmckerns merged commit 4f3fe3d into uqfoundation:master Jun 7, 2023
@mmckerns
Copy link
Member

mmckerns commented Jun 7, 2023

I'll take it as is. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants