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

chibby0ne

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

https://bugs.python.org/issue36161

chibby0ne

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

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

@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

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

chibby0ne

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.

bedevere-bot

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

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

Thanks for making the requested changes!

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

benjaminp

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

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

I have made the requested changes; please review again

bedevere-bot

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

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

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.

bedevere-bot

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

I have made the requested changes; please review again

bedevere-bot

Thanks for making the requested changes!

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

chibby0ne

@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

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

benjaminp

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)~~ 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.

Conversation

chibby0ne commented Jul 20, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chibby0ne commented Jul 20, 2019

Uh oh!

chibby0ne commented Jul 20, 2019

Uh oh!

mangrisano left a comment

Choose a reason for hiding this comment

Uh oh!

chibby0ne commented Jul 20, 2019

Uh oh!

chibby0ne commented Aug 1, 2019

Uh oh!

chibby0ne commented Aug 7, 2019

Uh oh!

benjaminp left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 11, 2019

Uh oh!

chibby0ne commented Sep 14, 2019

Uh oh!

bedevere-bot commented Sep 14, 2019

Uh oh!

benjaminp commented Sep 24, 2019

Uh oh!

chibby0ne commented Sep 28, 2019

Uh oh!

chibby0ne commented Sep 28, 2019

Uh oh!

bedevere-bot commented Sep 28, 2019

Uh oh!

benjaminp left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 28, 2019

Uh oh!

benjaminp left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 5, 2019

Uh oh!

chibby0ne commented Oct 5, 2019

Uh oh!

bedevere-bot commented Oct 5, 2019

Uh oh!

chibby0ne commented Oct 5, 2019

Uh oh!

chibby0ne commented Oct 7, 2019

Uh oh!

benjaminp commented Oct 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chibby0ne commented Jul 20, 2019 •

edited by bedevere-bot

Loading