[issue42778] Add follow_symlinks=True to {os.path,Path}.samefile

[Tom Hale]

New submission from Tom Hale :

The os.path and Path implementations of samefile() do not allow comparisons of 
symbolic links:

% mkdir empty && chdir empty
% ln -s non-existant broken
% ln broken lnbroken
% ls -i # Show inode numbers
19325632 broken@  19325632 lnbroken@
% Yup, they are the same file... but...
% python -c 'import os; print(os.path.samefile("lnbroken", "broken", 
follow_symlinks=False))'
Traceback (most recent call last):
  File "", line 1, in 
TypeError: samefile() got an unexpected keyword argument 'follow_symlinks'
% python -c 'import os; print(os.path.samefile("lnbroken", "broken"))'
Traceback (most recent call last):
  File "", line 1, in 
  File "/usr/lib/python3.8/genericpath.py", line 100, in samefile
s1 = os.stat(f1)
FileNotFoundError: [Errno 2] No such file or directory: 'lnbroken'
%

Both samefile()s use os.stat under the hood, but neither allow setting  
os.stat()'s `follow_symlinks` parameter.

https://docs.python.org/3/library/os.html#os.stat

https://docs.python.org/3/library/os.path.html#os.path.samefile

https://docs.python.org/3/library/pathlib.html#pathlib.Path.samefile

--
components: Library (Lib)
messages: 383965
nosy: Tom Hale
priority: normal
severity: normal
status: open
title: Add follow_symlinks=True to {os.path,Path}.samefile
type: behavior
versions: Python 3.9

___
Python tracker 
<https://bugs.python.org/issue42778>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42778] Add follow_symlinks=True parameter to both os.path.samefile() and Path.samefile()

[Tom Hale]

Tom Hale  added the comment:

In summary:

The underlying os.stat() takes a follow_symlinks=True parameter but it can't be 
set to False when trying to samefile() two symbolic links.

--
title: Add follow_symlinks=True to {os.path,Path}.samefile -> Add 
follow_symlinks=True parameter to both os.path.samefile() and Path.samefile()

___
Python tracker 
<https://bugs.python.org/issue42778>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Add race-free os.link and os.symlink wrapper / helper

[Tom Hale]

Tom Hale  added the comment:

Related issue found in testing:

bpo-42778 Add follow_symlinks=True parameter to both os.path.samefile() and 
Path.samefile()

--

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41134] distutils.dir_util.copy_tree FileExistsError when updating symlinks

[Tom Hale]

New submission from Tom Hale :

Here is a minimal test case:

==
#!/bin/bash

cd /tmp || exit 1

dir=test-copy_tree
src=$dir/src
dst=$dir/dst

mkdir -p "$src"
touch "$src"/file
ln -s file "$src/symlink"

python -c "from distutils.dir_util import copy_tree;
copy_tree('$src', '$dst', preserve_symlinks=1, update=1);
copy_tree('$src', '$dst', preserve_symlinks=1, update=1);"

rm -r "$dir"

==

Traceback (most recent call last):
  File "", line 3, in 
  File "/usr/lib/python3.8/distutils/dir_util.py", line 152, in copy_tree
os.symlink(link_dest, dst_name)
FileExistsError: [Errno 17] File exists: 'file' -> 'test-copy_tree/dst/symlink'

==


Related:
=

This issue will likely be resolved via:

bpo-36656 Add race-free os.link and os.symlink wrapper / helper

https://bugs.python.org/issue36656
(WIP under discussion at python-mentor)


Prior art:
===
https://stackoverflow.com/questions/53090360/python-distutils-copy-tree-fails-to-update-if-there-are-symlinks

--
messages: 372449
nosy: Tom Hale
priority: normal
severity: normal
status: open
title: distutils.dir_util.copy_tree FileExistsError when updating symlinks

___
Python tracker 
<https://bugs.python.org/issue41134>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Add race-free os.link and os.symlink wrapper / helper

[Tom Hale]

Tom Hale  added the comment:

Related:

bpo-41134 distutils.dir_util.copy_tree FileExistsError when updating symlinks


WIP update:

I am just about to ask for feedback on my proposed solution at 
core-mentors...@python.org

--
title: Please add race-free os.link and os.symlink wrapper / helper -> Add 
race-free os.link and os.symlink wrapper / helper

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41134] distutils.dir_util.copy_tree FileExistsError when updating symlinks

[Tom Hale]

Change by Tom Hale :


--
keywords: +patch
pull_requests: +20651
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/14464

___
Python tracker 
<https://bugs.python.org/issue41134>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Race conditions due to os.link and os.symlink POSIX specification

[Tom Hale]

Change by Tom Hale :


--
title: Allow os.symlink(src, target, force=True) to prevent race conditions -> 
Race conditions due to os.link and os.symlink POSIX specification

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Race conditions due to os.link and os.symlink POSIX specification

[Tom Hale]

Tom Hale  added the comment:

Serhiy wrote

> Detected problem is better than non-detected problem.

I agree. I also assert that no problem (via a shutil wrapper) is better than a 
detected problem which may not be handled.

While it's up to the programmer to handle exceptions, it's only systems 
programmers who will realise that there is a sometimes-occurring exception 
which should be handled.

So most people won't handle it. They don't know that they need to.

The shutil os.link and os.symlink wrappers under discussion on python-ideas 
prevent non-system-programmers from needing to know about the race-condition 
case (and correctly handling it).

If something is difficult to get right, then let's put it in shutil and save 
everyone reinventing the wheel.

3 specific cases in which an atomic link replacement would be useful are listed 
here:

https://code.activestate.com/lists/python-ideas/56195/

--

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Please add race-free os.link and os.symlink wrapper / helper

[Tom Hale]

Tom Hale  added the comment:

I've created a PR here:

https://github.com/python/cpython/pull/14464

Only shutil.symlink is currently implemented.

Feedback sought from Windows users.

@Michael.Felt please note that `overwrite=False` is the default.

@taleinat I hope that the new implementation addresses the infinite loop 
concern.

Please be both thorough and gentle - this is my first PR.

--

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Allow os.symlink(src, target, force=True) to prevent race conditions

[Tom Hale]

New submission from Tom Hale :

I cannot find a race-condition-free way to force overwrite an existing symlink.

os.symlink() requires that the target does not exist, meaning that it could be 
created via race condition the two workaround solutions that I've seen:

1. Unlink existing symlink (could be recreated, causing following symlink to 
fail)

2. Create a new temporary symlink, then overwrite target (temp could be changed 
between creation and replace.

The additional gotcha with the safer (because the attack filename is unknown) 
option (2) is that replace() may fail if the two files are on separate 
filesystems.

I suggest an additional `force=` argument to os.symlink(), defaulting to 
`False` for backward compatibility, but allowing atomic overwriting of a 
symlink when set to `True`.

I would be willing to look into a PR for this.

Prior art:  https://stackoverflow.com/a/55742015/5353461

--
messages: 340474
nosy: Tom Hale
priority: normal
severity: normal
status: open
title: Allow os.symlink(src, target, force=True) to prevent race conditions
versions: Python 3.7

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Allow os.symlink(src, target, force=True) to prevent race conditions

[Tom Hale]

Tom Hale  added the comment:

The most correct work-around I believe exists is:

(updates at: https://stackoverflow.com/a/55742015/5353461)

def symlink_force(target, link_name):
'''
Create a symbolic link pointing to target named link_name.
Overwrite target if it exists.
'''

# os.replace may fail if files are on different filesystems.
# Therefore, use the directory of target
link_dir = os.path.dirname(target)

# os.symlink requires that the target does NOT exist.
# Avoid race condition of file creation between mktemp and symlink:
while True:
temp_pathname = tempfile.mktemp(suffix='.tmp', \
prefix='symlink_force_tmp-', dir=link_dir)
try:
os.symlink(target, temp_pathname)
break  # Success, exit loop
except FileExistsError:
time.sleep(0.001)  # Prevent high load in pathological 
conditions
except:
raise
os.replace(temp_pathname, link_name)

An unlikely race condition still remains: the symlink created at the 
randomly-named `temp_path` could be modified between creation and 
rename/replacing the specified link name.

Suggestions for improvement welcome.

--
type:  -> security

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Allow os.symlink(src, target, force=True) to prevent race conditions

[Tom Hale]

Tom Hale  added the comment:

Yes, but by default (because of difficulty) people won't check for this case:

1. I delete existing symlink in order to recreate it
2. Attacker watching symlink finds it deleted and recreates it
3. I try to create symlink, and an unexpected exception is raised

--

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Allow os.symlink(src, target, force=True) to prevent race conditions

[Tom Hale]

Tom Hale  added the comment:

Thanks Toshio Kuratomi, I raised it on the mailing list at:

https://code.activestate.com/lists/python-ideas/55992/

--

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue36656] Allow os.symlink(src, target, force=True) to prevent race conditions

[Tom Hale]

Change by Tom Hale :


--
type: security -> enhancement

___
Python tracker 
<https://bugs.python.org/issue36656>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32001] @lru_cache needs to be called with ()

[Tom Hale]

New submission from Tom Hale :

This comes from a question I raised on StackOverflow:
https://stackoverflow.com/q/47218313/5353461

I've copied the text below

=
=


The [documentation for 
`lru_cache`](https://docs.python.org/3/library/functools.html#functools.lru_cache)
 gives the function definition:

> @functools.lru_cache(maxsize=128, typed=False)

This says to me that `maxsize` is optional.

However, it doesn't like being called without an argument:

Python 3.6.3 (default, Oct 24 2017, 14:48:20) 
[GCC 7.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import functools
>>> @functools.lru_cache
... def f(): ...
... 
Traceback (most recent call last):
  File "", line 1, in 
  File "/usr/lib/python3.6/functools.py", line 477, in lru_cache
raise TypeError('Expected maxsize to be an integer or None')
TypeError: Expected maxsize to be an integer or None
 >>> 

Calling with an argument is fine:

>>> @functools.lru_cache(8)
... def f(): ...
... 
>>> 

Am I misreading the documentation?

=
=

The answer presented this solution:

if callable(maxsize):
def decorating_function(user_function):
wrapper = _lru_cache_wrapper(user_function, maxsize, typed, 
_CacheInfo)
return update_wrapper(wrapper, user_function)
return decorating_function(max_size) # yes, max_size is the function in 
this case O:)


=
=

Would you accept a PR based on this solution?

--
messages: 306016
nosy: ataraxy
priority: normal
severity: normal
status: open
title: @lru_cache needs to be called with ()

___
Python tracker 
<https://bugs.python.org/issue32001>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com