bpo-36161: Use thread-safe function ttyname_r instead of unsafe one ttyname #14868

chibby0ne · 2019-07-20T00:58:46Z

Signed-off-by: Antonio Gutierrez chibby0ne@gmail.com

https://bugs.python.org/issue36161

chibby0ne · 2019-07-20T01:12:07Z

Hi all, I'm a new contributor and I've made a PR taking into account the discussion seen in the issue page.

For the ttyname_r(), I've used the buffer length of MAXPATHLEN, since I saw it used for several functions inside the same file, like: os.readlink, os.getcw, among others.

For the crypt_r() side, I've added a few unit tests one that raises the error and another that doesn't but could add more if needed.

This is how a raised error would look when passing an invalid salt:

>>> crypt.crypt('password', '__', raise_error=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/tmp/cpython/Lib/crypt.py", line 73, in crypt
    return _crypt.crypt(word, salt, raise_error)
OSError: [Errno 22] Invalid argument

chibby0ne · 2019-07-20T05:03:16Z

I don't understand the error in the MacOS Builder:

ERROR: test_invalid_salt_raises_error_false (test.test_crypt.CryptTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/vsts/agent/2.154.3/work/1/s/Lib/test/test_crypt.py", line 90, in test_invalid_salt_raises_error_false
    res = crypt.crypt('password', '__')
  File "/Users/vsts/agent/2.154.3/work/1/s/Lib/crypt.py", line 73, in crypt
    return _crypt.crypt(word, salt, raise_error)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xfd in position 3: invalid start byte

I don't understand since they are both unicode, and password is clearly utf-8 and the second argument are two _ underscore chars. Also the 0xfd byte is 'ý'.

If it were some codec issue wouldn't it fail in other OSes?

Likewise for the other added test.

mangrisano

Hi and thank you for this PR and for providing tests.

LGTM.

chibby0ne · 2019-07-20T18:03:47Z

@mangrisano Thanks for the approval. Unfortunately the CI is failing in MacOS. If you can, would you give me any pointers as to the cause of the issue? I truly don't understand it 😞

chibby0ne · 2019-08-01T18:30:44Z

Hi @mangrisano, and any others. Just a friendly ping to mention that this is ready to be merged.

chibby0ne · 2019-08-07T09:46:38Z

Hi all,
I need some help understanding why the MacOS test fails with that unicode error, specially since it passes for every other OS. I don't have access to a MacOS environment and have no way of testing this locally.

benjaminp

I don't understand these raise_error changes. Those don't seem related to changing the underlying library call.

benjaminp · 2019-09-11T16:37:28Z

Modules/posixmodule.c

@@ -2712,13 +2712,14 @@ static PyObject *
 os_ttyname_impl(PyObject *module, int fd)
 /*[clinic end generated code: output=c424d2e9d1cd636a input=9ff5a58b08115c55]*/
 {
-    char *ret;
+    int ret;
+    char buffer[MAXPATHLEN];


seems like the right buffer size is actually TTY_NAME_MAX

Correct. This macro wasn't mentioned in the version of the man pages of my local dev machine (i.e: man ttyname) but after googling this constant, it is indeed mentioned in man limits.h which is imported by Python.h in the base code.
Good catch! Thanks for the recommendation!

Modules/posixmodule.c

benjaminp · 2019-09-11T16:46:49Z

Modules/posixmodule.c


-    ret = ttyname(fd);
-    if (ret == NULL) {
+    ret = ttyname_r(fd, buffer, MAXPATHLEN);


Prefer sizeof(buffer)

bedevere-bot · 2019-09-11T16:47:50Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

chibby0ne · 2019-09-14T16:57:15Z

I don't understand these raise_error changes. Those don't seem related to changing the underlying library call.

Well according to the discussion in: https://bugs.python.org/issue36161, due to the fact that we are now returning the error code set by crypt_r (if any), this is a breaking change, so in order to maintain backwards compatible behavior (where regarless of any error it would return None), we add this boolean keyword argument raise_error.
But ofc I'm open to any suggestions 😄

I have made the requested changes; please review again

bedevere-bot · 2019-09-14T16:57:18Z

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

benjaminp · 2019-09-24T05:10:29Z

I don't understand these raise_error changes. Those don't seem related to changing the underlying library call.

Well according to the discussion in: https://bugs.python.org/issue36161, due to the fact that we are now returning the error code set by crypt_r (if any), this is a breaking change, so in order to maintain backwards compatible behavior (where regarless of any error it would return None), we add this boolean keyword argument raise_error.

While I agree it's technically a breaking change, I think not raising errors from a failed cryptographic function is severely buggy behavior. "Errors should never pass silently."

But ofc I'm open to any suggestions smile

I think errors should always be propagated from crypt or you should follow the suggestion of https://bugs.python.org/issue36161#msg351981 and simply drop the crypt changes.

chibby0ne · 2019-09-28T00:41:56Z

While I agree it's technically a breaking change, I think not raising errors from a failed cryptographic function is severely buggy behavior. "Errors should never pass silently."

Wise aphorism! I agree wholeheartedly.

I think errors should always be propagated from crypt or you should follow the suggestion of https://bugs.python.org/issue36161#msg351981 and simply drop the crypt changes.

Yeah it looks like this will be eventually deprecated and removed. I've decided to simply raise the error in accordingly, since it helps the user but also we are not over engineering future deprecated code.

chibby0ne · 2019-09-28T00:43:17Z

I have made the requested changes; please review again

bedevere-bot · 2019-09-28T00:43:19Z

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

benjaminp

okay, but now the tests are failng

bedevere-bot · 2019-09-28T21:48:27Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

benjaminp · 2019-10-02T02:14:59Z

Modules/posixmodule.c


-    ret = ttyname(fd);
-    if (ret == NULL) {
+#ifndef TTY_NAME_MAX    /* MacOS doesn't define this macro */


Huh, maybe it's better to use sysconf(_SC_TTY_NAME_MAX) for portability.

I wasn't sure about this because I saw the values were quite different between the ones used in OpenBSD and the ones used in Linux, but I agree is the more portable solution

benjaminp · 2019-10-02T02:17:36Z

Lib/crypt.py

+    result = None
+    try:
+        result = crypt('', salt)
+    except OSError:


Can you check that the errno of this error is EINVAL?

I think it would also be good to ensure that at least one method works.

Good thinking! Done! Thanks a lot 👍

benjaminp · 2019-10-02T02:18:47Z

Misc/NEWS.d/next/Library/2019-07-20-01-17-43.bpo-36161.Fzf-f9.rst

@@ -0,0 +1 @@
+Use thread-safe functions instead of unsafe ones (crypt, ttyname)


"Use ttyname_r instead of ttyname and crypt_r instead of crypt for thread safety."

I forgot to update this, because it was autogenerated from the first commit. Thanks a lot for your review 😉

benjaminp

There are really two logical changes in this PR: checking the error of crypt and using ttyname_r. It would make sense to split them into separate PRs I think.

benjaminp · 2019-10-05T00:22:38Z

Modules/posixmodule.c

        return posix_error();
    }
-    return PyUnicode_DecodeFSDefault(ret);
+    char *buffer = (char *) PyMem_RawMalloc(size);


Nit: no space between the cast and the function call.

benjaminp · 2019-10-05T00:22:55Z

Modules/posixmodule.c

+    int ret = ttyname_r(fd, buffer, size);
+    if (ret != 0) {
+        errno = ret;
+        return posix_error();


This branch now leaks buffer.

Good catch! Thanks ;)

benjaminp · 2019-10-05T00:23:50Z

Modules/posixmodule.c

+    PyObject *res;
+    res = PyUnicode_DecodeFSDefault(buffer);


Fold these two lines into one:

PyObject *res = PyUnicode_DecodeFSDefault(buffer);

benjaminp · 2019-10-05T00:24:52Z

Misc/NEWS.d/next/Library/2019-07-20-01-17-43.bpo-36161.Fzf-f9.rst

@@ -0,0 +1 @@
+For thread safetyy, use ``ttyname_r(3)`` instead of ``ttyname(3)`` and ``crypt_r(3)`` instead of ``crypt(3)``.


This is not correct, though, since you're not using crypt_r any more. :P

(I should have seen that last time I looked at the message.)

In that case will only leave this PR for the ttyname_r, and then create another one for just the error checking of crypt, and will change the news message accordingly.

bedevere-bot · 2019-10-05T00:26:49Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

* Use dynamically-allocated buffer for ttyname_r buffer * In case there is an error with the crypt/crypt_r then raise the error, instead of failing silently. * Check for EINVAL (invalid argument) in errno when adding not supported cryptographic methods, such as Blowfish in the case of glibc, but raise other errors. * Use _SC_TTY_MAX_LENGTH for TTY_NAME_MAX Signed-off-by: Antonio Gutierrez <chibby0ne@gmail.com>

chibby0ne · 2019-10-05T19:29:06Z

I have made the requested changes; please review again

bedevere-bot · 2019-10-05T19:29:10Z

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

chibby0ne · 2019-10-05T19:30:09Z

@benjaminp What do you think I should name the other PR? AFAIK the PR branch should match the name of the issue. Should I name fix-issue-36161_part2?

chibby0ne · 2019-10-07T08:26:17Z

Hi @benjaminp, the error check for crypt/crypt_r was moved to: #16599

benjaminp · 2019-10-09T02:20:03Z

Thank you for the PR.

…onGH-14868) Signed-off-by: Antonio Gutierrez <chibby0ne@gmail.com>

PR python#14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

PR #14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

…ython#128503) (cherry picked from commit e08b282) PR python#14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

…) (#128599) (cherry picked from commit e08b282) PR #14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

…#128503) PR python#14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

…) (#128598) (cherry picked from commit e08b282) PR #14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

the-knights-who-say-ni added the CLA signed label Jul 20, 2019

bedevere-bot added the awaiting review label Jul 20, 2019

mangrisano approved these changes Jul 20, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jul 20, 2019

chibby0ne force-pushed the fix-issue-36161 branch from 46a137a to 21deb92 Compare August 4, 2019 18:12

chibby0ne force-pushed the fix-issue-36161 branch from 21deb92 to 94f62fb Compare August 14, 2019 19:08

benjaminp requested changes Sep 11, 2019

View reviewed changes

bedevere-bot removed the awaiting core review label Sep 11, 2019

bedevere-bot added the awaiting changes label Sep 11, 2019

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 14, 2019

bedevere-bot requested a review from benjaminp September 14, 2019 16:57

benjaminp requested changes Sep 28, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Sep 28, 2019

benjaminp reviewed Oct 2, 2019

View reviewed changes

bedevere-bot requested a review from benjaminp October 4, 2019 18:34

benjaminp requested changes Oct 5, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Oct 5, 2019

chibby0ne force-pushed the fix-issue-36161 branch from c21c775 to 5799801 Compare October 5, 2019 19:27

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 5, 2019

bedevere-bot requested a review from benjaminp October 5, 2019 19:29

chibby0ne changed the title ~~bpo-36161: Use thread-safe functions instead of unsafe ones (crypt, ttyname)~~ bpo-36161: Use thread-safe function ttyname_r instead of unsafe one ttyname Oct 5, 2019

tweak NEWS

1614f7d

benjaminp merged commit 594e2ed into python:master Oct 9, 2019

bedevere-bot removed the awaiting change review label Oct 9, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

closes bpo-36161: Use thread-safe ttyname_r instead of ttyname. (pyth…

a59b58b

…onGH-14868) Signed-off-by: Antonio Gutierrez <chibby0ne@gmail.com>

glebm mentioned this pull request Dec 4, 2024

configure is checking for ttyname but code is using ttyname_r #127614

Closed

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Jan 4, 2025

pythongh-127614: Correctly check for ttyname_r() in configure

f25911a

PR python#14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

erlend-aasland mentioned this pull request Jan 4, 2025

gh-127614: Correctly check for ttyname_r() in configure #128503

Merged

erlend-aasland added a commit that referenced this pull request Jan 7, 2025

gh-127614: Correctly check for ttyname_r() in configure (#128503)

e08b282

PR #14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

erlend-aasland mentioned this pull request Jan 7, 2025

[3.13] gh-127614: Correctly check for ttyname_r() in configure (#128503) #128598

Merged

erlend-aasland mentioned this pull request Jan 7, 2025

[3.12] gh-127614: Correctly check for ttyname_r() in configure (#128503) #128599

Merged

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025

pythongh-127614: Correctly check for ttyname_r() in configure (python…

ebc1cc2

…#128503) PR python#14868 replaced the ttyname() call with ttyname_r(), but the old check remained.

		@@ -0,0 +1 @@
		Use thread-safe functions instead of unsafe ones (crypt, ttyname)

		@@ -0,0 +1 @@
		For thread safetyy, use ``ttyname_r(3)`` instead of ``ttyname(3)`` and ``crypt_r(3)`` instead of ``crypt(3)``.

bpo-36161: Use thread-safe function ttyname_r instead of unsafe one ttyname #14868

bpo-36161: Use thread-safe function ttyname_r instead of unsafe one ttyname #14868

Conversation

chibby0ne commented Jul 20, 2019 • edited by bedevere-bot Loading

chibby0ne commented Jul 20, 2019

chibby0ne commented Jul 20, 2019

mangrisano left a comment

Choose a reason for hiding this comment

chibby0ne commented Jul 20, 2019

chibby0ne commented Aug 1, 2019

chibby0ne commented Aug 7, 2019

benjaminp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 11, 2019

chibby0ne commented Sep 14, 2019

bedevere-bot commented Sep 14, 2019

benjaminp commented Sep 24, 2019

chibby0ne commented Sep 28, 2019

chibby0ne commented Sep 28, 2019

bedevere-bot commented Sep 28, 2019

benjaminp left a comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 5, 2019

chibby0ne commented Oct 5, 2019

bedevere-bot commented Oct 5, 2019

chibby0ne commented Oct 5, 2019

chibby0ne commented Oct 7, 2019

benjaminp commented Oct 9, 2019

chibby0ne commented Jul 20, 2019 •

edited by bedevere-bot

Loading