FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-9566: Fixed _ssl module warnings by segevfiner · Pull Request #2495 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-9566: Fixed _ssl module warnings - #2495

Merged
zooba merged 3 commits into
python:masterfrom
segevfiner:bpo-9566-ssl-warnings
Jul 26, 2017
ConversationCommits (3)ChecksFiles changed
Merged

bpo-9566: Fixed _ssl module warnings#2495
zooba merged 3 commits into
python:masterfrom
segevfiner:bpo-9566-ssl-warnings

Conversation

segevfiner commented Jun 29, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Split from #2492 on request by @Haypo

The following remain:

  • ..\Modules\_ssl.c(2947): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data
    The Py_buffer can theoretically be larger... But I'm not sure that the protocol even supports that...
    Should this be casted away or fixed some other way?
  • ..\Modules\_ssl.c(4158): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of data
    Someone can theoretically pass a buffer big enough... But I doubt such a buffer is going to be written at once...
    Should this be casted away...?

https://bugs.python.org/issue9566

mention-bot commented Jun 29, 2017

Copy link
Copy Markdown

@segevfiner, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pitrou, @tiran and @serhiy-storchaka to be potential reviewers.

segevfiner mentioned this pull request Jun 29, 2017
Merged
4 of 5 tasks
vstinner requested changes Jun 30, 2017
View reviewed changes
Comment thread Modules/_ssl.c
cipher_id = SSL_CIPHER_get_id(cipher);
SSL_CIPHER_description(cipher, buf, sizeof(buf) - 1);
len = strlen(buf);
len = (int)strlen(buf);

vstinner Jun 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

buf is 512 bytes long and always ends with a null byte, so the downcast is safe.

zooba Jul 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Right, and by explicitly stating that we are intentionally downcasting, someone who is reading the code can recognize that without having to do the flow analysis themselves. (Also, the compiler will recognize it too, and stop showing a warning.)

Comment thread Modules/_ssl.c Outdated
PyObject *result;

avail = BIO_ctrl_pending(self->bio);
avail = (int)BIO_ctrl_pending(self->bio);

vstinner Jun 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I dislike this downcast. I would prefer to use a larger integer type.

https://linux.die.net/man/3/bio_ctrl_pending says that the return type is size_t, it's unsigned. We may have to check < PY_SSIZE_T_MAX before downcasting to Py_ssize_t.

segevfiner Jun 30, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

BIO_read takes an int so we can only really only read as much as an int can hold without calling BIO_read in a loop. I replaced this with a Py_MIN which will make us read up to INT_MAX.

vstinner commented Jun 30, 2017

Copy link
Copy Markdown
Member 
..\Modules\_ssl.c(2947): warning C4244: '=': conversion from 'Py_ssize_t' to 'int', possible loss of data
The Py_buffer can theoretically be larger... But I'm not sure that the protocol even supports that...
Should this be casted away or fixed some other way?

_ssl__SSLContext__set_alpn_protocols_impl(). You must check for overflow manually: "if (len > INT_MAX) raise an exception;".

..\Modules\_ssl.c(4158): warning C4244: 'function': conversion from 'Py_ssize_t' to 'int', possible loss of data
Someone can theoretically pass a buffer big enough...

_ssl_MemoryBIO_write_impl(): you cannot, there is "if (b->len > INT_MAX) { ... }" a few lines before, so the downcast is safe. Make it explicit with a "(int)" cast.

segevfiner commented Jun 30, 2017

Copy link
Copy Markdown
Contributor Author

@Haypo

_ssl__SSLContext__set_alpn_protocols_impl(). You must check for overflow manually: "if (len > INT_MAX) raise an exception;".

PySSLContext.alpn_protocols_len was actually an int while it is treated as an unsigned int everywhere, so I changed it for correctness.

segevfiner changed the title [WIP] bpo-9566: Fixed some _ssl module warnings bpo-9566: Fixed some _ssl module warnings Jun 30, 2017
segevfiner changed the title bpo-9566: Fixed some _ssl module warnings bpo-9566: Fixed _ssl module warnings Jun 30, 2017
zooba approved these changes Jul 26, 2017
View reviewed changes
Comment thread Modules/_ssl.c
cipher_id = SSL_CIPHER_get_id(cipher);
SSL_CIPHER_description(cipher, buf, sizeof(buf) - 1);
len = strlen(buf);
len = (int)strlen(buf);

zooba Jul 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Right, and by explicitly stating that we are intentionally downcasting, someone who is reading the code can recognize that without having to do the flow analysis themselves. (Also, the compiler will recognize it too, and stop showing a warning.)

zooba commented Jul 26, 2017

Copy link
Copy Markdown
Member

The changes requested by @Haypo have been made, so I'm merging.

zooba merged commit 5cff637 into python:master Jul 26, 2017
segevfiner deleted the bpo-9566-ssl-warnings branch July 26, 2017 22:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner requested changes
zooba zooba approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL