bpo-9566: Fixed _ssl module warnings #2495
Conversation
|
@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. |
Modules/_ssl.c
Outdated
| @@ -1555,7 +1555,7 @@ cipher_to_dict(const SSL_CIPHER *cipher) | |||
| cipher_protocol = SSL_CIPHER_get_version(cipher); | |||
| cipher_id = SSL_CIPHER_get_id(cipher); | |||
| SSL_CIPHER_description(cipher, buf, sizeof(buf) - 1); | |||
| len = strlen(buf); | |||
| len = (int)strlen(buf); | |||
There was a problem hiding this comment.
buf is 512 bytes long and always ends with a null byte, so the downcast is safe.
There was a problem hiding this comment.
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.)
Modules/_ssl.c
Outdated
| @@ -4109,7 +4109,7 @@ _ssl_MemoryBIO_read_impl(PySSLMemoryBIO *self, int len) | |||
| int avail, nbytes; | |||
| PyObject *result; | |||
|
|
|||
| avail = BIO_ctrl_pending(self->bio); | |||
| avail = (int)BIO_ctrl_pending(self->bio); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
_ssl__SSLContext__set_alpn_protocols_impl(). You must check for overflow manually: "if (len > INT_MAX) raise an exception;".
_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
changed the title
segevfiner
changed the title
Modules/_ssl.c
Outdated
| @@ -1555,7 +1555,7 @@ cipher_to_dict(const SSL_CIPHER *cipher) | |||
| cipher_protocol = SSL_CIPHER_get_version(cipher); | |||
| cipher_id = SSL_CIPHER_get_id(cipher); | |||
| SSL_CIPHER_description(cipher, buf, sizeof(buf) - 1); | |||
| len = strlen(buf); | |||
| len = (int)strlen(buf); | |||
There was a problem hiding this comment.
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.)
|
The changes requested by @Haypo have been made, so I'm merging. |
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 dataThe 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 dataSomeone 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