Python's interpreter doesn't keep proper file permissions after
importing library. See the fallowing:

mk@laptop ~ $ echo "key='top secret'" > key.py
mk@laptop ~ $ chmod 600 key.py 
mk@laptop ~ $ python
Python 2.4.4 (#1, Jan  8 2008, 21:22:16) 
[GCC 4.1.2 (Gentoo 4.1.2 p1.0.1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import key
>>> 
mk@laptop ~ $ ls -l key.py*
-rw------- 1 mk mk  17 II  8 20:09 key.py
-rw-r--r-- 1 mk mk 120 II  8 20:09 key.pyc
mk@laptop ~ $ 

So, interpreter creates 644 pyo file (visible for all) which contains
secret data from 600 py file.
I think it should keep the original permissions, someone can save a
important data (eg. SQL login/pwd into Django's settings.py) into
library and makes it visible for all by an accident.

You have a good use case for a change of behavior. We might change it
for Python 2.6 and 3.0.

A proper place would be Python/import.c:load_source_module().

BTW, I see you've removed this issue from bugs list printed at
http://bugs.python.org/ but everyone (including unlogged visitors) can
access it still via http://bugs.python.org/issue2051 ...

stocker81 wrote:

BTW, I see you've removed this issue from bugs list printed at
http://bugs.python.org/ but everyone (including unlogged visitors) can
access it still via http://bugs.python.org/issue2051 ...

I haven't removed it from the listing. Look under priority "high" a few
pages up.

Christian

Christian Heimes <report@bugs.python.org> wrote:

I haven't removed it from the listing. Look under priority "high" a few
pages up.

I'm sorry, my mistake.

Regards,
Marcin

Fixed in r61002

Thanks for your patch Alexander! Good work.

Since there was no regression test added for this, it appears to me that it is broken again now that we're using importlib.

It may be rather hard to fix given the limitations of the set_data API :(

On Aug 19, 2012 9:26 PM, "Nick Coghlan" <report@bugs.python.org> wrote:

Nick Coghlan added the comment:

Since there was no regression test added for this, it appears to me that
it is broken again now that we're using importlib.

It may be rather hard to fix given the limitations of the set_data API :(

From an API perspective it's tough, but realistically it should be possible
since the loaders have the path to the file that triggered the find as the
'path' attribute.

----------
assignee: facundobatista ->
nosy: +brett.cannon, georg.brandl, ncoghlan
priority: high -> release blocker
resolution: accepted ->
status: closed -> open
versions: +Python 3.3 -Python 2.6

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue2051\>

---

here's a test. I'll work on a patch when I get a chance (and no one's beaten me to it :).

So this boils down to set_data() not having a mode parameter, right? Alas, the obvious approach, adding it, breaks backward compatibility. The alternative is to use source_from_cache() in set_data() to get the source path, and then get the mode there. Of course, differentiating between the use cases for set_data() seems important there too. In short, "It may be rather hard to fix given the limitations of the set_data API".

Regardless, we'll need to be careful to use the loader's cache to avoid extra stat calls.

Also, we have to factor in bpo-6074 (basically do "mode | 0o600"). We'll need at least one more test to cover that (as Nick noted in that issue).

I have a patch that is close, but my eyes are getting a little heavy. Hopefully I'll have that up tomorrow.

I have a patch that works, and that I think may point the way to a replacement API for set_data in 3.4. Just running the full test suite before I check it in.

The test you posted definitely saved me a lot of time.

My version bypasses the cache, though. This isn't a disaster, since the main reason the cache is there is to speed up *failing* stat calls - this new one only incurs the hit when actually writing the bytecode file to disk, which should be lost in the noise of the actual IO write operation.

However, suggestions for improvement always welcome :)

Also, my patch precisely recreates 3.2 behaviour, so it will mean bpo-6074 may also affect 3.3. That isn't a regression, hence not a release blocker, but would still be good to get fixed for rc1.

(oops, transposed the digits in the checkin message)

New changeset 3a831a0a29c4 by Nick Coghlan in branch 'default':
Close bpo-2051: Permission bits are once again correctly copied from the source file to the cached bytecode file. Test by Eric Snow.
http://hg.python.org/cpython/rev/3a831a0a29c4

New changeset cfb11045fc8a by Nick Coghlan in branch 'default':
Close bpo-2051: Oops, transposed the digits in the issue number in the previous commit
http://hg.python.org/cpython/rev/cfb11045fc8a

My patch was very similar. _cache_bytecode() is a good addition. Good point about the cache, too.

I'm not convinced that source_path is the right thing to add to the API (even just for SourceFileLoader). I would have thought mode would have been more appropriate:

  def set_data(self, path, data, *, mode=0o666):

Then the "mode = _os.stat(source_path).st_mode" bit would get moved to _cache_bytecode().

My reasoning is that set_data() is useful to write any data relative to the Loader. The concrete use case in the stdlib is for writing the .pyc files. Otherwise I'm not certain why the idea of "source_path" should be associated with set_data(). On the other hand, the idea of "mode" likewise may not be univeral enough to enshrine in the API, but I'd think it's more so than source_path.

I was going to ask about backward compatability, but then I realized that SourceFileLoader is new in 3.3 (SourceLoader is new in 3.2). I'm also wondering why set_data() is not a part of the FileLoader API, but that's a question for another time (and version). :)

I agree with Eric: set_data() is meant to be generic so as to allow writing arbitrary data, not just bytecode files. So while accepting a mode argument makes sense and I'm fine with in terms of possible API change (in the future), having a source_path argument I'm not comfortable with. So I'm going to just take Eric's idea and commit the change. I'm also making the argument _mode just to make sure no one relies on it until we can discuss API changes in the context of Python 3.4 sine we will need to clean up the loader APIs to have abstract source/bytecode stuff that can play nicely with file-based APIs to allow for reading generic file assets once we have all of these little bugs worked out.

And as for why set_data() is not part of FileLoader, that's because it isn't necessary to load files. =) It's in SourceFileLoader purely to facilitate writing bytecode files, but set_data() is not used at all in terms of the actual loading of source code (or bytecode files for that matter).

New changeset 0c661c5632e0 by Brett Cannon in branch 'default':
Issue bpo-2051: Tweak last commit for this issue to pass in mode instead
http://hg.python.org/cpython/rev/0c661c5632e0