Skip to content

Use secure builtins standard module, instead of the __builtins__ #109

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

Merged
merged 3 commits into from
Jul 27, 2022

Conversation

0xsirsaif
Copy link
Contributor

Due to documentation, it seems that using __builtins__ directly is not reliable. Instead they were using the __builtin__ module which was renamed to builtins.

Having __builtins__ and __builtin__ both is clearly a bad idea.

[Python-Dev] __builtin__ vs __builtins__

I hope this helps, as I am just starting with open source contributions.

@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #109 (e98883b) into main (297e922) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #109   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          522       522           
  Branches        86        86           
=========================================
  Hits           522       522           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 297e922...e98883b. Read the comment docs.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for contributing.

Please update the docs example too.

try:
from devtools import debug
except ImportError:
pass
else:
__builtins__['debug'] = debug
setattr(builtins, "debug", debug)
Copy link
Owner

Choose a reason for hiding this comment

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

Single quotes please.

@samuelcolvin
Copy link
Owner

I'm still not sure that the link you sent means we should change this, and although the cli is new the docs example had been around for a long time and no one has complained it doesn't work. Definitely it's always worked for me.

Is there a more contemporary, clearer definition of which which we should use?

@0xsirsaif
Copy link
Contributor Author

Thank You for the reply.

The main point that encourages me to use builtins module rather than the others is the confusion I found between the difference between __builtin__ (the singular one) vs __builtins__.

As I understand, using __builtins__ was less secure/reliable than using __builtin__, then they rename the later to builtins. Also, using __builtins__ outside the __main__ module would return a dict which is not the case when using it inside the __main__ module.

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

You were right from here:

CPython implementation detail: Users should not touch builtins; it is strictly an implementation detail. Users wanting to override values in the builtins namespace should import the builtins module and modify its attributes appropriately.

With these small changes I think this is ready.

@0xsirsaif
Copy link
Contributor Author

Thanks, I will make these changes right now.

@samuelcolvin
Copy link
Owner

Thanks so much for the contribution 🎉 .

I'll probably wait a bit now to see if anyone reports errors with the new release, then make a new release (or forget until I'm reminded, cos that's life).

@samuelcolvin samuelcolvin merged commit 0909f01 into samuelcolvin:main Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants