Tidy up the BIO_ctrl implementations They're all written in this weird style without early returns, making it hard to tell what it's returning. In doing so, fix a bug where conn_ctrl / BIO_C_SET_CONNECT returned 1 when ptr == nullptr instead of 0. (Upstream returns 0.) Also noticed a bug in pair BIOs, but I'll fix that in the next CL. Change-Id: Ibacb7577babeab081a337143f15b8e905490fdef Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/78828 Commit-Queue: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/crypto/bio/bio_mem.cc b/crypto/bio/bio_mem.cc index 2814074..25d7ffe 100644 --- a/crypto/bio/bio_mem.cc +++ b/crypto/bio/bio_mem.cc
@@ -164,10 +164,7 @@ } static long mem_ctrl(BIO *bio, int cmd, long num, void *ptr) { - long ret = 1; - - BUF_MEM *b = (BUF_MEM *)bio->ptr; - + BUF_MEM *b = static_cast<BUF_MEM *>(bio->ptr); switch (cmd) { case BIO_CTRL_RESET: if (b->data != NULL) { @@ -180,52 +177,46 @@ b->length = 0; } } - break; + return 1; case BIO_CTRL_EOF: - ret = (long)(b->length == 0); - break; + return b->length == 0; case BIO_C_SET_BUF_MEM_EOF_RETURN: - bio->num = (int)num; - break; + bio->num = static_cast<int>(num); + return 1; case BIO_CTRL_INFO: - ret = (long)b->length; - if (ptr != NULL) { - char **pptr = reinterpret_cast<char **>(ptr); - *pptr = b->data; + if (ptr != nullptr) { + char **out = reinterpret_cast<char **>(ptr); + *out = b->data; } - break; + // This API can overflow on 64-bit Windows, where |long| is smaller than + // |ptrdiff_t|. |BIO_mem_contents| is the overflow-safe API. + return static_cast<long>(b->length); case BIO_C_SET_BUF_MEM: mem_free(bio); - bio->shutdown = (int)num; + bio->shutdown = static_cast<int>(num); bio->ptr = ptr; - break; + return 1; case BIO_C_GET_BUF_MEM_PTR: if (ptr != NULL) { - BUF_MEM **pptr = reinterpret_cast<BUF_MEM **>(ptr); - *pptr = b; + BUF_MEM **out = reinterpret_cast<BUF_MEM **>(ptr); + *out = b; } - break; + return 1; case BIO_CTRL_GET_CLOSE: - ret = (long)bio->shutdown; - break; + return bio->shutdown; case BIO_CTRL_SET_CLOSE: - bio->shutdown = (int)num; - break; - + bio->shutdown = static_cast<int>(num); + return 1; case BIO_CTRL_WPENDING: - ret = 0L; - break; + return 0; case BIO_CTRL_PENDING: - ret = (long)b->length; - break; + // TODO(crbug.com/412584975): This can overflow on 64-bit Windows. + return static_cast<long>(b->length); case BIO_CTRL_FLUSH: - ret = 1; - break; + return 1; default: - ret = 0; - break; + return 0; } - return ret; } static const BIO_METHOD mem_method = {
diff --git a/crypto/bio/connect.cc b/crypto/bio/connect.cc index 21f09b9..fe08cc7 100644 --- a/crypto/bio/connect.cc +++ b/crypto/bio/connect.cc
@@ -353,93 +353,75 @@ } static long conn_ctrl(BIO *bio, int cmd, long num, void *ptr) { - int *ip; - long ret = 1; - BIO_CONNECT *data; - - data = (BIO_CONNECT *)bio->ptr; - + BIO_CONNECT *data = static_cast<BIO_CONNECT *>(bio->ptr); switch (cmd) { case BIO_CTRL_RESET: - ret = 0; data->state = BIO_CONN_S_BEFORE; conn_close_socket(bio); bio->flags = 0; - break; + return 0; case BIO_C_DO_STATE_MACHINE: // use this one to start the connection if (data->state != BIO_CONN_S_OK) { - ret = (long)conn_state(bio, data); + return conn_state(bio, data); } else { - ret = 1; + return 1; } - break; case BIO_C_SET_CONNECT: - if (ptr != NULL) { - bio->init = 1; - if (num == 0) { - OPENSSL_free(data->param_hostname); - data->param_hostname = - OPENSSL_strdup(reinterpret_cast<const char *>(ptr)); - if (data->param_hostname == NULL) { - ret = 0; - } - } else if (num == 1) { - OPENSSL_free(data->param_port); - data->param_port = - OPENSSL_strdup(reinterpret_cast<const char *>(ptr)); - if (data->param_port == NULL) { - ret = 0; - } - } else { - ret = 0; - } + if (ptr == nullptr) { + return 0; } - break; + bio->init = 1; + if (num == 0) { + OPENSSL_free(data->param_hostname); + data->param_hostname = + OPENSSL_strdup(reinterpret_cast<const char *>(ptr)); + if (data->param_hostname == nullptr) { + return 0; + } + } else if (num == 1) { + OPENSSL_free(data->param_port); + data->param_port = OPENSSL_strdup(reinterpret_cast<const char *>(ptr)); + if (data->param_port == nullptr) { + return 0; + } + } else { + return 0; + } + return 1; case BIO_C_SET_NBIO: - data->nbio = (int)num; - break; + data->nbio = static_cast<int>(num); + return 1; case BIO_C_GET_FD: if (bio->init) { - ip = (int *)ptr; - if (ip != NULL) { - *ip = bio->num; + int *out = static_cast<int *>(ptr); + if (out != nullptr) { + *out = bio->num; } - ret = bio->num; + return bio->num; } else { - ret = -1; + return -1; } - break; case BIO_CTRL_GET_CLOSE: - ret = bio->shutdown; - break; + return bio->shutdown; case BIO_CTRL_SET_CLOSE: - bio->shutdown = (int)num; - break; - case BIO_CTRL_PENDING: - case BIO_CTRL_WPENDING: - ret = 0; - break; + bio->shutdown = static_cast<int>(num); + return 1; case BIO_CTRL_FLUSH: - break; + return 1; case BIO_CTRL_GET_CALLBACK: { - int (**fptr)(const BIO *bio, int state, int xret); - fptr = reinterpret_cast<decltype(fptr)>(ptr); + auto fptr = + reinterpret_cast<int (**)(const BIO *bio, int state, int xret)>(ptr); *fptr = data->info_callback; - } break; + return 1; + } default: - ret = 0; - break; + return 0; } - return ret; } static long conn_callback_ctrl(BIO *bio, int cmd, bio_info_cb fp) { - long ret = 1; - BIO_CONNECT *data; - - data = (BIO_CONNECT *)bio->ptr; - + BIO_CONNECT *data = static_cast<BIO_CONNECT *>(bio->ptr); switch (cmd) { case BIO_CTRL_SET_CALLBACK: // This is the actual type signature of |fp|. The caller is expected to @@ -454,12 +436,10 @@ data->info_callback = (int (*)(const struct bio_st *, int, int))fp; OPENSSL_CLANG_PRAGMA("clang diagnostic pop") OPENSSL_MSVC_PRAGMA(warning(pop)) - break; + return 1; default: - ret = 0; - break; + return 0; } - return ret; } BIO *BIO_new_connect(const char *hostname) {
diff --git a/crypto/bio/fd.cc b/crypto/bio/fd.cc index 45819d7..159a9de 100644 --- a/crypto/bio/fd.cc +++ b/crypto/bio/fd.cc
@@ -96,62 +96,47 @@ } static long fd_ctrl(BIO *b, int cmd, long num, void *ptr) { - long ret = 1; - int *ip; - switch (cmd) { case BIO_CTRL_RESET: num = 0; [[fallthrough]]; case BIO_C_FILE_SEEK: - ret = 0; if (b->init) { - ret = (long)BORINGSSL_LSEEK(b->num, num, SEEK_SET); + return (long)BORINGSSL_LSEEK(b->num, num, SEEK_SET); } - break; + return 0; case BIO_C_FILE_TELL: case BIO_CTRL_INFO: - ret = 0; if (b->init) { - ret = (long)BORINGSSL_LSEEK(b->num, 0, SEEK_CUR); + return (long)BORINGSSL_LSEEK(b->num, 0, SEEK_CUR); } - break; + return 0; case BIO_C_SET_FD: fd_free(b); - b->num = *((int *)ptr); - b->shutdown = (int)num; + b->num = *static_cast<int *>(ptr); + b->shutdown = static_cast<int>(num); b->init = 1; - break; + return 1; case BIO_C_GET_FD: if (b->init) { - ip = (int *)ptr; - if (ip != NULL) { - *ip = b->num; + int *out = static_cast<int *>(ptr); + if (out != nullptr) { + *out = b->num; } return b->num; } else { - ret = -1; + return -1; } - break; case BIO_CTRL_GET_CLOSE: - ret = b->shutdown; - break; + return b->shutdown; case BIO_CTRL_SET_CLOSE: - b->shutdown = (int)num; - break; - case BIO_CTRL_PENDING: - case BIO_CTRL_WPENDING: - ret = 0; - break; + b->shutdown = static_cast<int>(num); + return 1; case BIO_CTRL_FLUSH: - ret = 1; - break; + return 1; default: - ret = 0; - break; + return 0; } - - return ret; } static int fd_gets(BIO *bp, char *buf, int size) {
diff --git a/crypto/bio/file.cc b/crypto/bio/file.cc index 40ca725..e907c08 100644 --- a/crypto/bio/file.cc +++ b/crypto/bio/file.cc
@@ -138,24 +138,18 @@ } static long file_ctrl(BIO *b, int cmd, long num, void *ptr) { - long ret = 1; - FILE *fp = (FILE *)b->ptr; - FILE **fpp; - + FILE *fp = static_cast<FILE *>(b->ptr); switch (cmd) { case BIO_CTRL_RESET: num = 0; [[fallthrough]]; case BIO_C_FILE_SEEK: - ret = (long)fseek(fp, num, 0); - break; + return fseek(fp, num, 0); case BIO_CTRL_EOF: - ret = (long)feof(fp); - break; + return feof(fp); case BIO_C_FILE_TELL: case BIO_CTRL_INFO: - ret = ftell(fp); - break; + return ftell(fp); case BIO_C_SET_FILE_PTR: file_free(b); static_assert((BIO_CLOSE & BIO_FP_TEXT) == 0, @@ -169,13 +163,13 @@ _setmode(_fileno(reinterpret_cast<FILE *>(ptr)), _O_TEXT); } #endif - b->shutdown = (int)num & BIO_CLOSE; + b->shutdown = static_cast<int>(num) & BIO_CLOSE; b->ptr = ptr; b->init = 1; - break; + return 1; case BIO_C_SET_FILENAME: file_free(b); - b->shutdown = (int)num & BIO_CLOSE; + b->shutdown = static_cast<int>(num) & BIO_CLOSE; const char *mode; if (num & BIO_FP_APPEND) { if (num & BIO_FP_READ) { @@ -191,43 +185,35 @@ mode = "rb"; } else { OPENSSL_PUT_ERROR(BIO, BIO_R_BAD_FOPEN_MODE); - ret = 0; - break; + return 0; } fp = fopen_if_available(reinterpret_cast<const char *>(ptr), mode); - if (fp == NULL) { + if (fp == nullptr) { OPENSSL_PUT_SYSTEM_ERROR(); ERR_add_error_data(5, "fopen('", ptr, "','", mode, "')"); OPENSSL_PUT_ERROR(BIO, ERR_R_SYS_LIB); - ret = 0; - break; + return 0; } b->ptr = fp; b->init = 1; - break; + return 1; case BIO_C_GET_FILE_PTR: // the ptr parameter is actually a FILE ** in this case. - if (ptr != NULL) { - fpp = (FILE **)ptr; - *fpp = (FILE *)b->ptr; + if (ptr != nullptr) { + FILE **out = static_cast<FILE **>(ptr); + *out = fp; } - break; + return 1; case BIO_CTRL_GET_CLOSE: - ret = (long)b->shutdown; - break; + return b->shutdown; case BIO_CTRL_SET_CLOSE: - b->shutdown = (int)num; - break; + b->shutdown = static_cast<int>(num); + return 1; case BIO_CTRL_FLUSH: - ret = 0 == fflush((FILE *)b->ptr); - break; - case BIO_CTRL_WPENDING: - case BIO_CTRL_PENDING: + return fflush(fp) == 0; default: - ret = 0; - break; + return 0; } - return ret; } static int file_gets(BIO *bp, char *buf, int size) {
diff --git a/crypto/bio/pair.cc b/crypto/bio/pair.cc index d12ea9d..ac5a9c5 100644 --- a/crypto/bio/pair.cc +++ b/crypto/bio/pair.cc
@@ -313,33 +313,32 @@ } static long bio_ctrl(BIO *bio, int cmd, long num, void *ptr) { - long ret; struct bio_bio_st *b = reinterpret_cast<bio_bio_st *>(bio->ptr); - - assert(b != NULL); - + assert(b != nullptr); switch (cmd) { // Specific control codes first: case BIO_C_GET_WRITE_BUF_SIZE: - ret = (long)b->size; - break; + // TODO(crbug.com/412584975): This can overflow on 64-bit Windows. Do we + // need it? It implements |BIO_get_write_buf_size|, but we don't have the + // wrapper. + return static_cast<long>(b->size); case BIO_C_GET_WRITE_GUARANTEE: // How many bytes can the caller feed to the next write // without having to keep any? - if (b->peer == NULL || b->closed) { - ret = 0; - } else { - ret = (long)b->size - b->len; + if (b->peer == nullptr || b->closed) { + return 0; } - break; + // TODO(crbug.com/412584975): This can overflow on 64-bit Windows. + return static_cast<long>(b->size - b->len); case BIO_C_GET_READ_REQUEST: // If the peer unsuccessfully tried to read, how many bytes // were requested? (As with BIO_CTRL_PENDING, that number // can usually be treated as boolean.) - ret = (long)b->request; - break; + // + // TODO(crbug.com/412584975): This can overflow on 64-bit Windows. + return static_cast<long>(b->request); case BIO_C_RESET_READ_REQUEST: // Reset request. (Can be useful after read attempts @@ -347,64 +346,54 @@ // e.g. when probing SSL_read to see if any data is // available.) b->request = 0; - ret = 1; - break; + return 1; case BIO_C_SHUTDOWN_WR: // similar to shutdown(..., SHUT_WR) b->closed = 1; - ret = 1; - break; - + return 1; // Standard control codes: case BIO_CTRL_GET_CLOSE: - ret = bio->shutdown; - break; + return bio->shutdown; case BIO_CTRL_SET_CLOSE: - bio->shutdown = (int)num; - ret = 1; - break; + bio->shutdown = static_cast<int>(num); + return 1; case BIO_CTRL_PENDING: - if (b->peer != NULL) { + if (b->peer != nullptr) { struct bio_bio_st *peer_b = reinterpret_cast<bio_bio_st *>(b->peer->ptr); - ret = (long)peer_b->len; - } else { - ret = 0; + // TODO(crbug.com/412584975): This can overflow on 64-bit Windows. + return static_cast<long>(peer_b->len); } - break; + return 0; case BIO_CTRL_WPENDING: - ret = 0; - if (b->buf != NULL) { - ret = (long)b->len; + if (b->buf == nullptr) { + return 0; } - break; + // TODO(crbug.com/412584975): This can overflow on 64-bit Windows. + return static_cast<long>(b->len); case BIO_CTRL_FLUSH: - ret = 1; - break; + return 1; case BIO_CTRL_EOF: { BIO *other_bio = reinterpret_cast<BIO *>(ptr); - if (other_bio) { struct bio_bio_st *other_b = reinterpret_cast<bio_bio_st *>(other_bio->ptr); - assert(other_b != NULL); - ret = other_b->len == 0 && other_b->closed; - } else { - ret = 1; + assert(other_b != nullptr); + return other_b->len == 0 && other_b->closed; } - } break; + return 1; + } default: - ret = 0; + return 0; } - return ret; }
diff --git a/crypto/bio/socket.cc b/crypto/bio/socket.cc index 672c9b5..d1066a7 100644 --- a/crypto/bio/socket.cc +++ b/crypto/bio/socket.cc
@@ -83,41 +83,32 @@ } static long sock_ctrl(BIO *b, int cmd, long num, void *ptr) { - long ret = 1; - int *ip; - switch (cmd) { case BIO_C_SET_FD: sock_free(b); - b->num = *((int *)ptr); - b->shutdown = (int)num; + b->num = *static_cast<int *>(ptr); + b->shutdown = static_cast<int>(num); b->init = 1; - break; + return 1; case BIO_C_GET_FD: if (b->init) { - ip = (int *)ptr; - if (ip != NULL) { - *ip = b->num; + int *out = static_cast<int*>(ptr); + if (out != nullptr) { + *out = b->num; } - ret = b->num; - } else { - ret = -1; + return b->num; } - break; + return -1; case BIO_CTRL_GET_CLOSE: - ret = b->shutdown; - break; + return b->shutdown; case BIO_CTRL_SET_CLOSE: - b->shutdown = (int)num; - break; + b->shutdown = static_cast<int>(num); + return 1; case BIO_CTRL_FLUSH: - ret = 1; - break; + return 1; default: - ret = 0; - break; + return 0; } - return ret; } static const BIO_METHOD methods_sockp = {