Tidy up error handling for sockets vs fds
On Windows, sockets and fds are different, so we need to be a little
carefully. The fd functions (which are really a userspace construct
inside the libc) report errors by writing to errno:
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-170
While the socket functions (which are really thin wrappers over Windows
HANDLEs) use WSAGetLastError:
https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-recv
https://learn.microsoft.com/en-us/windows/win32/winsock/error-codes-errno-h-errno-and-wsagetlasterror-2
Moreover, the error values are different, so we shouldn't mix them
together:
https://learn.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2
https://learn.microsoft.com/en-us/cpp/c-runtime-library/errno-constants?view=msvc-170
Finally, by borrowing OpenSSL's distinct OPENSSL_NO_SOCK and
OPENSSL_NO_POSIX_IO options, we arguably should account for all
combinations of one or the other being missing. (Ugh.) To account for
that, I've moved bio_fd_should_retry into its own file that isn't
conditioned on anything. It only depends on <errno.h>, which is part of
the C standard library, and used elsewhere already.
Change-Id: I0519d7d68c32062e1220ffca0ab57a9cac9f7e5f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61729
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt
index d1cc984..f0c892f 100644
--- a/crypto/CMakeLists.txt
+++ b/crypto/CMakeLists.txt
@@ -96,6 +96,7 @@
bio/bio.c
bio/bio_mem.c
bio/connect.c
+ bio/errno.c
bio/fd.c
bio/file.c
bio/hexdump.c
diff --git a/crypto/bio/connect.c b/crypto/bio/connect.c
index cf52dde..d48d14e 100644
--- a/crypto/bio/connect.c
+++ b/crypto/bio/connect.c
@@ -233,7 +233,7 @@
BIO_clear_retry_flags(bio);
ret = connect(bio->num, (struct sockaddr*) &c->them, c->them_length);
if (ret < 0) {
- if (bio_fd_should_retry(ret)) {
+ if (bio_socket_should_retry(ret)) {
BIO_set_flags(bio, (BIO_FLAGS_IO_SPECIAL | BIO_FLAGS_SHOULD_RETRY));
c->state = BIO_CONN_S_BLOCKED_CONNECT;
bio->retry_reason = BIO_RR_CONNECT;
@@ -252,7 +252,7 @@
case BIO_CONN_S_BLOCKED_CONNECT:
i = bio_sock_error(bio->num);
if (i) {
- if (bio_fd_should_retry(ret)) {
+ if (bio_socket_should_retry(ret)) {
BIO_set_flags(bio, (BIO_FLAGS_IO_SPECIAL | BIO_FLAGS_SHOULD_RETRY));
c->state = BIO_CONN_S_BLOCKED_CONNECT;
bio->retry_reason = BIO_RR_CONNECT;
@@ -366,7 +366,7 @@
ret = (int)recv(bio->num, out, out_len, 0);
BIO_clear_retry_flags(bio);
if (ret <= 0) {
- if (bio_fd_should_retry(ret)) {
+ if (bio_socket_should_retry(ret)) {
BIO_set_retry_read(bio);
}
}
@@ -390,7 +390,7 @@
ret = (int)send(bio->num, in, in_len, 0);
BIO_clear_retry_flags(bio);
if (ret <= 0) {
- if (bio_fd_should_retry(ret)) {
+ if (bio_socket_should_retry(ret)) {
BIO_set_retry_write(bio);
}
}
diff --git a/crypto/bio/errno.c b/crypto/bio/errno.c
new file mode 100644
index 0000000..901ea0c
--- /dev/null
+++ b/crypto/bio/errno.c
@@ -0,0 +1,92 @@
+/* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
+ * All rights reserved.
+ *
+ * This package is an SSL implementation written
+ * by Eric Young (eay@cryptsoft.com).
+ * The implementation was written so as to conform with Netscapes SSL.
+ *
+ * This library is free for commercial and non-commercial use as long as
+ * the following conditions are aheared to. The following conditions
+ * apply to all code found in this distribution, be it the RC4, RSA,
+ * lhash, DES, etc., code; not just the SSL code. The SSL documentation
+ * included with this distribution is covered by the same copyright terms
+ * except that the holder is Tim Hudson (tjh@cryptsoft.com).
+ *
+ * Copyright remains Eric Young's, and as such any Copyright notices in
+ * the code are not to be removed.
+ * If this package is used in a product, Eric Young should be given attribution
+ * as the author of the parts of the library used.
+ * This can be in the form of a textual message at program startup or
+ * in documentation (online or textual) provided with the package.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * 3. All advertising materials mentioning features or use of this software
+ * must display the following acknowledgement:
+ * "This product includes cryptographic software written by
+ * Eric Young (eay@cryptsoft.com)"
+ * The word 'cryptographic' can be left out if the rouines from the library
+ * being used are not cryptographic related :-).
+ * 4. If you include any Windows specific code (or a derivative thereof) from
+ * the apps directory (application code) you must include an acknowledgement:
+ * "This product includes software written by Tim Hudson (tjh@cryptsoft.com)"
+ *
+ * THIS SOFTWARE IS PROVIDED BY ERIC YOUNG ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ *
+ * The licence and distribution terms for any publically available version or
+ * derivative of this code cannot be changed. i.e. this code cannot simply be
+ * copied and put under another distribution licence
+ * [including the GNU Public Licence.] */
+
+#include <openssl/bio.h>
+
+#include <errno.h>
+
+#include "internal.h"
+
+
+int bio_errno_should_retry(int return_value) {
+ if (return_value != -1) {
+ return 0;
+ }
+
+ return
+#ifdef EWOULDBLOCK
+ errno == EWOULDBLOCK ||
+#endif
+#ifdef ENOTCONN
+ errno == ENOTCONN ||
+#endif
+#ifdef EINTR
+ errno == EINTR ||
+#endif
+#ifdef EAGAIN
+ errno == EAGAIN ||
+#endif
+#ifdef EPROTO
+ errno == EPROTO ||
+#endif
+#ifdef EINPROGRESS
+ errno == EINPROGRESS ||
+#endif
+#ifdef EALREADY
+ errno == EALREADY ||
+#endif
+ 0;
+}
diff --git a/crypto/bio/fd.c b/crypto/bio/fd.c
index c128e81..3b38db8 100644
--- a/crypto/bio/fd.c
+++ b/crypto/bio/fd.c
@@ -65,9 +65,6 @@
#include <unistd.h>
#else
#include <io.h>
-OPENSSL_MSVC_PRAGMA(warning(push, 3))
-#include <windows.h>
-OPENSSL_MSVC_PRAGMA(warning(pop))
#endif
#include <openssl/err.h>
@@ -77,59 +74,18 @@
#include "../internal.h"
-static int bio_fd_non_fatal_error(int err) {
- if (
-#ifdef EWOULDBLOCK
- err == EWOULDBLOCK ||
-#endif
-#ifdef WSAEWOULDBLOCK
- err == WSAEWOULDBLOCK ||
-#endif
-#ifdef ENOTCONN
- err == ENOTCONN ||
-#endif
-#ifdef EINTR
- err == EINTR ||
-#endif
-#ifdef EAGAIN
- err == EAGAIN ||
-#endif
-#ifdef EPROTO
- err == EPROTO ||
-#endif
-#ifdef EINPROGRESS
- err == EINPROGRESS ||
-#endif
-#ifdef EALREADY
- err == EALREADY ||
-#endif
- 0) {
- return 1;
- }
- return 0;
-}
-
#if defined(OPENSSL_WINDOWS)
- #define BORINGSSL_ERRNO (int)GetLastError()
#define BORINGSSL_CLOSE _close
#define BORINGSSL_LSEEK _lseek
#define BORINGSSL_READ _read
#define BORINGSSL_WRITE _write
#else
- #define BORINGSSL_ERRNO errno
#define BORINGSSL_CLOSE close
#define BORINGSSL_LSEEK lseek
#define BORINGSSL_READ read
#define BORINGSSL_WRITE write
#endif
-int bio_fd_should_retry(int i) {
- if (i == -1) {
- return bio_fd_non_fatal_error(BORINGSSL_ERRNO);
- }
- return 0;
-}
-
BIO *BIO_new_fd(int fd, int close_flag) {
BIO *ret = BIO_new(BIO_s_fd());
if (ret == NULL) {
@@ -161,7 +117,7 @@
ret = (int)BORINGSSL_READ(b->num, out, outl);
BIO_clear_retry_flags(b);
if (ret <= 0) {
- if (bio_fd_should_retry(ret)) {
+ if (bio_errno_should_retry(ret)) {
BIO_set_retry_read(b);
}
}
@@ -173,7 +129,7 @@
int ret = (int)BORINGSSL_WRITE(b->num, in, inl);
BIO_clear_retry_flags(b);
if (ret <= 0) {
- if (bio_fd_should_retry(ret)) {
+ if (bio_errno_should_retry(ret)) {
BIO_set_retry_write(b);
}
}
diff --git a/crypto/bio/internal.h b/crypto/bio/internal.h
index 8ed27da..ea2aa6a 100644
--- a/crypto/bio/internal.h
+++ b/crypto/bio/internal.h
@@ -78,7 +78,9 @@
#endif
-// BIO_ip_and_port_to_socket_and_addr creates a socket and fills in |*out_addr|
+#if !defined(OPENSSL_NO_SOCK)
+
+// bio_ip_and_port_to_socket_and_addr creates a socket and fills in |*out_addr|
// and |*out_addr_length| with the correct values for connecting to |hostname|
// on |port_str|. It returns one on success or zero on error.
int bio_ip_and_port_to_socket_and_addr(int *out_sock,
@@ -87,21 +89,27 @@
const char *hostname,
const char *port_str);
-// BIO_socket_nbio sets whether |sock| is non-blocking. It returns one on
+// bio_socket_nbio sets whether |sock| is non-blocking. It returns one on
// success and zero otherwise.
int bio_socket_nbio(int sock, int on);
-// BIO_clear_socket_error clears the last system socket error.
+// bio_clear_socket_error clears the last system socket error.
//
// TODO(fork): remove all callers of this.
void bio_clear_socket_error(void);
-// BIO_sock_error returns the last socket error on |sock|.
+// bio_sock_error returns the last socket error on |sock|.
int bio_sock_error(int sock);
-// BIO_fd_should_retry returns non-zero if |return_value| indicates an error
+// bio_socket_should_retry returns non-zero if |return_value| indicates an error
+// and the last socket error indicates that it's non-fatal.
+int bio_socket_should_retry(int return_value);
+
+#endif // !OPENSSL_NO_SOCK
+
+// bio_errno_should_retry returns non-zero if |return_value| indicates an error
// and |errno| indicates that it's non-fatal.
-int bio_fd_should_retry(int return_value);
+int bio_errno_should_retry(int return_value);
#if defined(__cplusplus)
diff --git a/crypto/bio/socket.c b/crypto/bio/socket.c
index 6084e96..72c772f 100644
--- a/crypto/bio/socket.c
+++ b/crypto/bio/socket.c
@@ -104,7 +104,7 @@
#endif
BIO_clear_retry_flags(b);
if (ret <= 0) {
- if (bio_fd_should_retry(ret)) {
+ if (bio_socket_should_retry(ret)) {
BIO_set_retry_read(b);
}
}
@@ -120,7 +120,7 @@
#endif
BIO_clear_retry_flags(b);
if (ret <= 0) {
- if (bio_fd_should_retry(ret)) {
+ if (bio_socket_should_retry(ret)) {
BIO_set_retry_write(b);
}
}
diff --git a/crypto/bio/socket_helper.c b/crypto/bio/socket_helper.c
index 366996c..fd8181f 100644
--- a/crypto/bio/socket_helper.c
+++ b/crypto/bio/socket_helper.c
@@ -121,4 +121,13 @@
return error;
}
+int bio_socket_should_retry(int return_value) {
+#if defined(OPENSSL_WINDOWS)
+ return return_value == -1 && WSAGetLastError() == WSAEWOULDBLOCK;
+#else
+ // On POSIX platforms, sockets and fds are the same.
+ return bio_errno_should_retry(return_value);
+#endif
+}
+
#endif // OPENSSL_NO_SOCK
