-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
Thread-unsafe libc functions #127081
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
Comments
This reliably reproduces the race in the repro-127081-sockets.pyimport socket
import sys
import threading
ITERATIONS=100
services = [
('echo', 7),
('ftp', 21),
('ssh', 22),
('telnet', 23),
('smtp', 25)]
protos = [
('icmp', 1),
('ipv4', 4),
('igp', 9),
('udp', 17),
('ipv6', 41)]
def getservbyname(barrier, service, port):
barrier.wait()
assert socket.getservbyname(service) == port
def getservbyport(barrier, service, port):
barrier.wait()
assert socket.getservbyport(port) == service
def getprotobyname(barrier, proto, port):
barrier.wait()
assert socket.getprotobyname(proto) == port
def run_threads(threads):
for t in threads:
t.start()
for t in threads:
t.join()
def test_getservbyname():
barrier = threading.Barrier(len(services))
threads = [threading.Thread(target=getservbyname, args=(barrier, service, port))
for service, port in services]
run_threads(threads)
def test_getservbyport():
barrier = threading.Barrier(len(services))
threads = [threading.Thread(target=getservbyport, args=(barrier, service, port))
for service, port in services]
run_threads(threads)
def test_getprotobyname():
barrier = threading.Barrier(len(protos))
threads = [threading.Thread(target=getprotobyname, args=(barrier, proto, port))
for proto, port in protos]
run_threads(threads)
bailing = threading.Event()
old_except_hook = None
def bail(args):
bailing.set()
old_except_hook(args)
if __name__ == '__main__':
old_except_hook = threading.excepthook
threading.excepthook = bail
for _ in range(ITERATIONS):
if bailing.is_set():
break
test_getservbyname()
test_getservbyport()
test_getprotobyname() |
Add configure tests and defines for getservbyname_r, getservbyport_r, and getprotobyname_r. Use these if available, otherwise fallback to the thread-unsafe variants. Add a unit test to exercise getprotobyname, which is currently untested. TODO: - Are there any platforms which define the unsafe variants but not the re-entrant ones? If not we can simplify the #ifdef hell somewhat. - Do the re-entrant functions have the same signature on all platforms? - These changes follow the existing code's practice: allocate a fixed-size (and overly large) buffer, and don't properly handle the error case if it is too small. Should this be fixed? If so should existing code also be fixed?
Reproductions for repro-127081-pwd.pyimport pwd
import threading
import unittest
N=5
ITERATIONS=1000
pwds = set(pwd.getpwall())
def getpwall(barrier):
barrier.wait()
assert pwds == set(pwd.getpwall())
def run_threads(threads):
for t in threads:
t.start()
for t in threads:
t.join()
if __name__ == '__main__':
for _ in range(ITERATIONS):
barrier = threading.Barrier(N)
threads = [threading.Thread(target=getpwall, args=(barrier,)) for _ in range(N)]
run_threads(threads) repro-127081-dbm.pyimport dbm
import random
import tempfile
import threading
from pathlib import Path
N=5
ITERATIONS=1000
def run_threads(threads):
for t in threads:
t.start()
for t in threads:
t.join()
def traverse(barrier, db):
barrier.wait()
for key in db.keys():
try:
assert db[key] == key
except KeyError:
pass
def inserter(barrier, db):
barrier.wait()
for _ in range(N):
ch = chr(ord('a') + random.randrange(26))
db[ch] = ch
def deleter(barrier, db):
barrier.wait()
for _ in range(N):
key = random.choice(db.keys())
try:
del db[key]
except KeyError:
pass
def defaulter(barrier, db):
barrier.wait()
for _ in range(N):
ch = chr(ord('a') + random.randrange(26))
result = db.setdefault(ch, ch)
# TODO: Is this a gdbm bug?
if isinstance(result, bytes):
result = result.decode()
assert result == ch, f"{result} != {ch}"
def checker(barrier, db):
barrier.wait()
for _ in range(N * 10):
assert len(db) >= 0
key = random.choice(db.keys())
key in db
def init_db(path):
with dbm.open(path, 'c') as db:
for s in ['foo', 'bar', 'baz']:
db[s] = s
return path
def test_db(path):
for _ in range(ITERATIONS):
barrier = threading.Barrier(N + 4)
with dbm.open(path, 'w') as db:
args = (barrier, db)
threads = [threading.Thread(target=traverse, args=args) for _ in range(N)]
threads.append(threading.Thread(target=inserter, args=args))
threads.append(threading.Thread(target=deleter, args=args))
threads.append(threading.Thread(target=defaulter, args=args))
threads.append(threading.Thread(target=checker, args=args))
run_threads(threads)
if __name__ == '__main__':
# dbm.sqlite3 only allows accessing DBs from the same thread that opened
# them (and enforces this)
for backend in ['dbm.gnu', 'dbm.ndbm']:
dbm.whichdb = lambda x: backend
with tempfile.TemporaryDirectory() as dir:
path = Path(dir) / "test-db"
init_db(path)
test_db(path) I can't reproduce a problem with The issues with |
The libc setpwent, getpwent, and endpwent functions are not thread-safe. Protect them with mutexs in free-threading builds.
The dbm_* functions are not thread-safe, naturally. Add critical sections to protect their use.
The getlogin function is not thread-safe: replace with getlogin_r where available. Note that this function is untested (unit test is skipped with a note it caused CI failures as behaviour differs between NIX environments).
See also #74667, which is a dup of this (or vice versa) specifically regarding the |
The dbm_* functions are not thread-safe, naturally. Add critical sections to protect their use.
The libc setpwent, getpwent, and endpwent functions are not thread-safe. Protect them with mutexs in free-threading builds.
Add configure tests and defines for getservbyname_r, getservbyport_r, and getprotobyname_r. Use these if available, otherwise fallback to the thread-unsafe variants. Add a unit test to exercise getprotobyname, which is currently untested. TODO: - Are there any platforms which define the unsafe variants but not the re-entrant ones? If not we can simplify the #ifdef hell somewhat. - Do the re-entrant functions have the same signature on all platforms? - These changes follow the existing code's practice: allocate a fixed-size (and overly large) buffer, and don't properly handle the error case if it is too small. Should this be fixed? If so should existing code also be fixed?
The getlogin function is not thread-safe: replace with getlogin_r where available. Note that this function is untested (unit test is skipped with a note it caused CI failures as behaviour differs between NIX environments).
Bug report
There are a a few non thread-safe libc functions used in Python that can be an issue for free threading, isolated subinterpreters, or sometimes even with the GIL.
In #126316, @vstinner fixed the use
setgrent
/getgrent
. It's probably a good time to look for other similar issues.clang-tidy
clang-tidy
has aconcurrency-mt-unsafe
check that looks for "known-to-be-unsafe functions".clang-tidy notes
Prerequisites: install
clang-tidy-18
andbear
.Unsafe libc functions
localeconv()
: not thread-safe, see glibc's manual. Isnl_langinfo
a substitue?setlocale
setpwent
,getpwent
, andendpwent
inpwdmodule.c
. These are similar togrpmodule.c
and can likely be addressed the same way.getservbyname
,getservbyport
,getprotobyname
inModules/socketmodule.c
: usegetservbyname_r
, etc.? Note these thread-safety issues affect the default build because we release the GIL around the relevant calls.dbm_open
,dbm_close
, etc. inModules/_dbmmodule.c
getlogin
: usegetlogin_r
if available?Unfixable by us?
getenv
,setenv
,unsetenv
,putenv
: environment modification isn't thread-safe andgetenv
is used extensively in other C libraries, so putting a lock around our accesses doesn't do much. (Might finally be fixed in glibc, see https://inbox.sourceware.org/libc-alpha/cover.1722193092.git.fweimer@redhat.com/).login_tty
: not sure it matters aslogin_tty
is usually called after afork()
Safe due to our usage
getc_unlocked
, safe because we use it within aflockfile()
call.mbrtowc()
- safe as long as the passed inmbstate_t *
is non-NULL, which is the case in CPython.Safe in glibc
These functions are flagged by clang-tidy because they are not guaranteed to be safe by POSIX, but they are safe in glibc. It'd be nice to verify that they are safe in other libc implementations. I don't think it's worth changing them:
dlerror()
strerror()
: see https://man7.org/linux/man-pages/man3/strerror.3.htmlsystem()
: see https://man7.org/linux/man-pages/man3/system.3.htmlreaddir(DIR *dirp)
: see https://man7.org/linux/man-pages/man3/readdir.3.html: "...in modern implementations (including the glibc implementation), concurrent calls to readdir() that specify different directory streams are thread-safe."wcstombs
: see https://man7.org/linux/man-pages/man3/wcstombs.3.htmlnl_langinfo
: https://www.man7.org/linux/man-pages/man3/nl_langinfo.3.html (as long as locale doesn't change)tcsendbreak
,tcflow
: see https://www.man7.org/linux/man-pages/man3/termios.3.html#ATTRIBUTESsetlogmask
: safe since glibc 2.33strsignal
: not documented as thread-safe, but uses glibc uses TLS internally since 2.32Other
exit()
: apparently concurrent calls toexit()
are not thread-safe, but I don't think it matters for our usages.ptsname()
: we already useptsname_r()
, but the static analyzer gets confusedLinked PRs
*pwent
calls #132748dbm
objects #132749get{proto,serv}by{name,port}
#132750getlogin_r
if available #132751The text was updated successfully, but these errors were encountered: