Skip to content

bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} #7081

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.

Sign up for GitHub

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 9 commits into from
Sep 7, 2018
Merged

bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} #7081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
454bc0c
Release GIL for `grp` and `pwd` single methods.
william-gr May 23, 2018
e9a92f1
Address @serhiy-storchaka review
william-gr May 24, 2018
dcc6af6
Address @serhiy-storchaka and @vstinner reviews
william-gr Jun 6, 2018
491f1f8
Address @serhiy-storchaka review
william-gr Jun 6, 2018
5cccd68
Use PyErr_NoMemory()
william-gr Jun 10, 2018
649fb60
Use while instead of do as advised
william-gr Jun 10, 2018
e59d2c1
GIL is not always released, reword NEWS entry.
william-gr Sep 6, 2018
874339e
Address reviews from @vstinner
william-gr Sep 6, 2018
b02311e
Move Realloc and remove Malloc so we always check if it succeeded
william-gr Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Library/2018-05-23-17-07-54.bpo-33625.nzQgD8.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Release GIL on `grp.getgrnam`, `grp.getgrgid`, `pwd.getpwnam` and
`pwd.getpwuid` if reentrant variants of these functions are available.
Patch by William Grzybowski.
103 changes: 97 additions & 6 deletions Modules/grpmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ static PyStructSequence_Desc struct_group_type_desc = {
static int initialized;
static PyTypeObject StructGrpType;

#define DEFAULT_BUFFER_SIZE 1024

static PyObject *
mkgrent(struct group *p)
{
Expand Down Expand Up @@ -96,7 +98,9 @@ static PyObject *
grp_getgrgid_impl(PyObject *module, PyObject *id)
/*[clinic end generated code: output=30797c289504a1ba input=15fa0e2ccf5cda25]*/
{
PyObject *py_int_id;
PyObject *py_int_id, *retval = NULL;
int nomem = 0;
char *buf = NULL, *buf2 = NULL;
gid_t gid;
struct group *p;

Expand All @@ -119,16 +123,60 @@ grp_getgrgid_impl(PyObject *module, PyObject *id)
}
Py_DECREF(py_int_id);
}
#ifdef HAVE_GETGRGID_R
Py_BEGIN_ALLOW_THREADS
int status;
Py_ssize_t bufsize;
struct group grp;

bufsize = sysconf(_SC_GETGR_R_SIZE_MAX);
if (bufsize == -1) {
bufsize = DEFAULT_BUFFER_SIZE;
}

while (1) {
buf2 = PyMem_RawRealloc(buf, bufsize);
if (buf2 == NULL) {
p = NULL;
nomem = 1;
break;
}
buf = buf2;
status = getgrgid_r(gid, &grp, buf, bufsize, &p);
if (status != 0) {
p = NULL;
}
if (p != NULL || status != ERANGE) {
break;
}
if (bufsize > (PY_SSIZE_T_MAX >> 1)) {
nomem = 1;
break;
}
bufsize <<= 1;
}

if ((p = getgrgid(gid)) == NULL) {
Py_END_ALLOW_THREADS
#else
p = getgrgid(gid);
#endif
if (p == NULL) {
PyMem_RawFree(buf);
if (nomem == 1) {
return PyErr_NoMemory();
}
PyObject *gid_obj = _PyLong_FromGid(gid);
if (gid_obj == NULL)
return NULL;
PyErr_Format(PyExc_KeyError, "getgrgid(): gid not found: %S", gid_obj);
Py_DECREF(gid_obj);
return NULL;
}
return mkgrent(p);
retval = mkgrent(p);
#ifdef HAVE_GETGRGID_R
PyMem_RawFree(buf);
#endif
return retval;
}

/*[clinic input]
Expand All @@ -145,7 +193,8 @@ static PyObject *
grp_getgrnam_impl(PyObject *module, PyObject *name)
/*[clinic end generated code: output=67905086f403c21c input=08ded29affa3c863]*/
{
char *name_chars;
char *buf = NULL, *buf2 = NULL, *name_chars;
int nomem = 0;
struct group *p;
PyObject *bytes, *retval = NULL;

Expand All @@ -154,13 +203,55 @@ grp_getgrnam_impl(PyObject *module, PyObject *name)
/* check for embedded null bytes */
if (PyBytes_AsStringAndSize(bytes, &name_chars, NULL) == -1)
goto out;
#ifdef HAVE_GETGRNAM_R
Py_BEGIN_ALLOW_THREADS
int status;
Py_ssize_t bufsize;
struct group grp;

bufsize = sysconf(_SC_GETGR_R_SIZE_MAX);
if (bufsize == -1) {
bufsize = DEFAULT_BUFFER_SIZE;
}

while(1) {
buf2 = PyMem_RawRealloc(buf, bufsize);
if (buf2 == NULL) {
p = NULL;
nomem = 1;
break;
}
buf = buf2;
status = getgrnam_r(name_chars, &grp, buf, bufsize, &p);
if (status != 0) {
p = NULL;
}
if (p != NULL || status != ERANGE) {
break;
}
if (bufsize > (PY_SSIZE_T_MAX >> 1)) {
nomem = 1;
break;
}
bufsize <<= 1;
}

if ((p = getgrnam(name_chars)) == NULL) {
PyErr_Format(PyExc_KeyError, "getgrnam(): name not found: %s", name_chars);
Py_END_ALLOW_THREADS
#else
p = getgrnam(name_chars);
#endif
if (p == NULL) {
if (nomem == 1) {
PyErr_NoMemory();
}
else {
PyErr_Format(PyExc_KeyError, "getgrnam(): name not found: %s", name_chars);
Copy link
Member

Choose a reason for hiding this comment

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

Hum, I see a bug which is not related to your change, but while we modify these functions, it would be nice to fix it.

name_chars is encoded to the filesystem encoding, whereas %s decodes it from UTF-8. If the filesystem encoding is not UTF-8, you get mojibake.

The fix is simple: use %S format and pass name: name is always a Unicode string.

Copy link
Member

Choose a reason for hiding this comment

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

If name is always a Unicode string, you can use %U.

It may be worth to use %R for the case if the name contains invisible characters or trailing whitespaces.

Copy link
Member

Choose a reason for hiding this comment

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

See https://bugs.python.org/issue34604 :-)

}
goto out;
}
retval = mkgrent(p);
out:
PyMem_RawFree(buf);
Py_DECREF(bytes);
return retval;
}
Expand Down
104 changes: 98 additions & 6 deletions Modules/pwdmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ exception is raised if the entry asked for cannot be found.");
static int initialized;
static PyTypeObject StructPwdType;

#define DEFAULT_BUFFER_SIZE 1024

static void
sets(PyObject *v, int i, const char* val)
{
Expand Down Expand Up @@ -116,16 +118,59 @@ static PyObject *
pwd_getpwuid(PyObject *module, PyObject *uidobj)
/*[clinic end generated code: output=c4ee1d4d429b86c4 input=ae64d507a1c6d3e8]*/
{
PyObject *retval = NULL;
uid_t uid;
int nomem = 0;
struct passwd *p;
char *buf = NULL, *buf2 = NULL;

if (!_Py_Uid_Converter(uidobj, &uid)) {
if (PyErr_ExceptionMatches(PyExc_OverflowError))
PyErr_Format(PyExc_KeyError,
"getpwuid(): uid not found");
return NULL;
}
if ((p = getpwuid(uid)) == NULL) {
#ifdef HAVE_GETPWUID_R
Py_BEGIN_ALLOW_THREADS
int status;
Py_ssize_t bufsize;
struct passwd pwd;

bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
if (bufsize == -1) {
bufsize = DEFAULT_BUFFER_SIZE;
}

while(1) {
buf2 = PyMem_RawRealloc(buf, bufsize);
if (buf2 == NULL) {
nomem = 1;
break;
}
buf = buf2;
status = getpwuid_r(uid, &pwd, buf, bufsize, &p);
if (status != 0) {
p = NULL;
}
if (p != NULL || status != ERANGE) {
break;
}
if (bufsize > (PY_SSIZE_T_MAX >> 1)) {
nomem = 1;
break;
}
bufsize <<= 1;
}

Py_END_ALLOW_THREADS
#else
p = getpwuid(uid);
#endif
if (p == NULL) {
PyMem_RawFree(buf);
if (nomem == 1) {
return PyErr_NoMemory();
}
PyObject *uid_obj = _PyLong_FromUid(uid);
if (uid_obj == NULL)
return NULL;
Expand All @@ -134,7 +179,11 @@ pwd_getpwuid(PyObject *module, PyObject *uidobj)
Py_DECREF(uid_obj);
return NULL;
}
return mkpwent(p);
retval = mkpwent(p);
#ifdef HAVE_GETPWUID_R
PyMem_RawFree(buf);
#endif
return retval;
}

/*[clinic input]
Expand All @@ -152,7 +201,8 @@ static PyObject *
pwd_getpwnam_impl(PyObject *module, PyObject *arg)
/*[clinic end generated code: output=6abeee92430e43d2 input=d5f7e700919b02d3]*/
{
char *name;
char *buf = NULL, *buf2 = NULL, *name;
int nomem = 0;
struct passwd *p;
PyObject *bytes, *retval = NULL;

Expand All @@ -161,13 +211,55 @@ pwd_getpwnam_impl(PyObject *module, PyObject *arg)
/* check for embedded null bytes */
if (PyBytes_AsStringAndSize(bytes, &name, NULL) == -1)
goto out;
if ((p = getpwnam(name)) == NULL) {
PyErr_Format(PyExc_KeyError,
"getpwnam(): name not found: %s", name);
#ifdef HAVE_GETPWNAM_R
Py_BEGIN_ALLOW_THREADS
int status;
Py_ssize_t bufsize;
struct passwd pwd;

bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
if (bufsize == -1) {
bufsize = DEFAULT_BUFFER_SIZE;
}

while(1) {
buf2 = PyMem_RawRealloc(buf, bufsize);
if (buf2 == NULL) {
nomem = 1;
break;
}
buf = buf2;
status = getpwnam_r(name, &pwd, buf, bufsize, &p);
if (status != 0) {
p = NULL;
}
if (p != NULL || status != ERANGE) {
break;
}
if (bufsize > (PY_SSIZE_T_MAX >> 1)) {
nomem = 1;
break;
}
bufsize <<= 1;
}

Py_END_ALLOW_THREADS
#else
p = getpwnam(name);
#endif
if (p == NULL) {
if (nomem == 1) {
PyErr_NoMemory();
}
else {
PyErr_Format(PyExc_KeyError,
"getpwnam(): name not found: %s", name);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment: please use the %S format here, but if you modify this function, would you mind to also fix the name of the parameter? Rename "arg" to "name", as in Doc/library/pwd.rst. Maybe rename the char* name to name_chars, as in grp_getgrnam_impl().

}
goto out;
}
retval = mkpwent(p);
out:
PyMem_RawFree(buf);
Py_DECREF(bytes);
return retval;
}
Expand Down
18 changes: 16 additions & 2 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,7 @@ infodir
docdir
oldincludedir
includedir
runstatedir
localstatedir
sharedstatedir
sysconfdir
Expand Down Expand Up @@ -891,6 +892,7 @@ datadir='${datarootdir}'
sysconfdir='${prefix}/etc'
sharedstatedir='${prefix}/com'
localstatedir='${prefix}/var'
runstatedir='${localstatedir}/run'
includedir='${prefix}/include'
oldincludedir='/usr/include'
docdir='${datarootdir}/doc/${PACKAGE_TARNAME}'
Expand Down Expand Up @@ -1143,6 +1145,15 @@ do
| -silent | --silent | --silen | --sile | --sil)
silent=yes ;;

-runstatedir | --runstatedir | --runstatedi | --runstated \
| --runstate | --runstat | --runsta | --runst | --runs \
| --run | --ru | --r)
ac_prev=runstatedir ;;
-runstatedir=* | --runstatedir=* | --runstatedi=* | --runstated=* \
| --runstate=* | --runstat=* | --runsta=* | --runst=* | --runs=* \
| --run=* | --ru=* | --r=*)
runstatedir=$ac_optarg ;;

-sbindir | --sbindir | --sbindi | --sbind | --sbin | --sbi | --sb)
ac_prev=sbindir ;;
-sbindir=* | --sbindir=* | --sbindi=* | --sbind=* | --sbin=* \
Expand Down Expand Up @@ -1280,7 +1291,7 @@ fi
for ac_var in exec_prefix prefix bindir sbindir libexecdir datarootdir \
datadir sysconfdir sharedstatedir localstatedir includedir \
oldincludedir docdir infodir htmldir dvidir pdfdir psdir \
libdir localedir mandir
libdir localedir mandir runstatedir
do
eval ac_val=\$$ac_var
# Remove trailing slashes.
Expand Down Expand Up @@ -1433,6 +1444,7 @@ Fine tuning of the installation directories:
--sysconfdir=DIR read-only single-machine data [PREFIX/etc]
--sharedstatedir=DIR modifiable architecture-independent data [PREFIX/com]
--localstatedir=DIR modifiable single-machine data [PREFIX/var]
--runstatedir=DIR modifiable per-process data [LOCALSTATEDIR/run]
--libdir=DIR object code libraries [EPREFIX/lib]
--includedir=DIR C header files [PREFIX/include]
--oldincludedir=DIR C header files for non-gcc [/usr/include]
Expand Down Expand Up @@ -11263,8 +11275,9 @@ for ac_func in alarm accept4 setitimer getitimer bind_textdomain_codeset chown \
clock confstr ctermid dup3 execv faccessat fchmod fchmodat fchown fchownat \
fexecve fdopendir fork fpathconf fstatat ftime ftruncate futimesat \
futimens futimes gai_strerror getentropy \
getgrgid_r getgrnam_r \
getgrouplist getgroups getlogin getloadavg getpeername getpgid getpid \
getpriority getresuid getresgid getpwent getspnam getspent getsid getwd \
getpriority getresuid getresgid getpwent getpwnam_r getpwuid_r getspnam getspent getsid getwd \
if_nameindex \
initgroups kill killpg lchmod lchown lockf linkat lstat lutimes mmap \
memrchr mbrtowc mkdirat mkfifo \
Expand Down Expand Up @@ -13696,6 +13709,7 @@ fi




# checks for system services
# (none yet)

Expand Down
3 changes: 2 additions & 1 deletion configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3445,8 +3445,9 @@ AC_CHECK_FUNCS(alarm accept4 setitimer getitimer bind_textdomain_codeset chown \
clock confstr ctermid dup3 execv faccessat fchmod fchmodat fchown fchownat \
fexecve fdopendir fork fpathconf fstatat ftime ftruncate futimesat \
futimens futimes gai_strerror getentropy \
getgrgid_r getgrnam_r \
getgrouplist getgroups getlogin getloadavg getpeername getpgid getpid \
getpriority getresuid getresgid getpwent getspnam getspent getsid getwd \
getpriority getresuid getresgid getpwent getpwnam_r getpwuid_r getspnam getspent getsid getwd \
if_nameindex \
initgroups kill killpg lchmod lchown lockf linkat lstat lutimes mmap \
memrchr mbrtowc mkdirat mkfifo \
Expand Down
Loading