Repository: valkey-io/valkey
File: src/tls.c (C)

Code before:

    /* Check whether addr is an IP address, if not, use the value for Server Name Indication */
    if (inet_pton(AF_INET, addr, addr_buf) != 1 && inet_pton(AF_INET6, addr, addr_buf) != 1) {
        SSL_set_tlsext_host_name(conn->ssl, addr);
    }

    /* Initiate Socket connection first */
    if (connectionTypeTcp()->connect(conn_, addr, port, src_addr, connect_handler) == C_ERR) return C_ERR;

    /* Return now, once the socket is connected we'll initiate
     * TLS connection from the event handler.
     */
    return C_OK;
}

static int connTLSWrite(connection *conn_, const void *data, size_t data_len) {
    tls_connection *conn = (tls_connection *)conn_;
    int ret;

    if (conn->c.state != CONN_STATE_CONNECTED) return -1;
    ERR_clear_error();
    /* In case when last write failed due to some internal reason, retry has to provide
     * at least the same amount of bytes (https://docs.openssl.org/master/man3/SSL_write).
     * If that condition is not met, OpenSSL will return "SSL routines::bad length".
     * When this assertion gets hit, caller has to be fixed to provide required bytes. */
    debugServerAssert(data_len >= conn->last_failed_write_data_len);
    ret = SSL_write(conn->ssl, data, data_len);
    conn->last_failed_write_data_len = ret <= 0 ? data_len : 0;
    return updateStateAfterSSLIO(conn, ret, 1);
}

static int connTLSWritev(connection *conn_, const struct iovec *iov, int iovcnt) {
    tls_connection *conn = (tls_connection *)conn_;
    if (iovcnt == 1) return connTLSWrite(conn_, iov[0].iov_base, iov[0].iov_len);

    /* Accumulate the amount of bytes of each buffer and check if it exceeds NET_MAX_WRITES_PER_EVENT. */
    size_t iov_bytes_len = 0;
    for (int i = 0; i < iovcnt; i++) {
        iov_bytes_len += iov[i].iov_len;
        if (iov_bytes_len > NET_MAX_WRITES_PER_EVENT) break;
    }

    /* In case the amount of all buffers is greater than NET_MAX_WRITES_PER_EVENT,
     * it might not worth doing so much memory copying to reduce system calls,
     * therefore, invoke connTLSWrite() multiple times to avoid memory copies.
     * However, in case when last write failed we still have to repeat sending last_failed_write_data_len
     * bytes. Because of openssl implementation we cannot repeat sending writes with length smaller than
     * the last failed write (https://docs.openssl.org/master/man3/SSL_write) so in case the first io buffer
     * does not provide at least the same amount of bytes as previous failed write, we will have to fallback to
     * memory copy to a static buffer before calling SSL_write. */
    if (iov_bytes_len > NET_MAX_WRITES_PER_EVENT && iovcnt > 0 && iov[0].iov_len >= conn->last_failed_write_data_len) {

Review comment (bug):
So this debug assert would still leave us with the current issue (causing connection to be disconnected) in production I think we should still return -1 after the debug assert

Code after:

    /* Check whether addr is an IP address, if not, use the value for Server Name Indication */
    if (inet_pton(AF_INET, addr, addr_buf) != 1 && inet_pton(AF_INET6, addr, addr_buf) != 1) {
        SSL_set_tlsext_host_name(conn->ssl, addr);
    }

    /* Initiate Socket connection first */
    if (connectionTypeTcp()->connect(conn_, addr, port, src_addr, connect_handler) == C_ERR) return C_ERR;

    /* Return now, once the socket is connected we'll initiate
     * TLS connection from the event handler.
     */
    return C_OK;
}

static int connTLSWrite(connection *conn_, const void *data, size_t data_len) {
    tls_connection *conn = (tls_connection *)conn_;
    int ret;

    if (conn->c.state != CONN_STATE_CONNECTED) return -1;
    ERR_clear_error();
    /* In case when last write failed due to some internal reason, retry has to provide
     * at least the same amount of bytes (https://docs.openssl.org/master/man3/SSL_write).
     * If that condition is not met, OpenSSL will return "SSL routines::bad length".
     * Currently we only suspect this can happen during primary cron sending '\n'
     * indication to the replica, so we silently return from this function without
     * impacting the connection state. */
    if (data_len < conn->last_failed_write_data_len) {
        // TODO: place debugAssert for this case once the known issue described is resolved
        return -1;
    }
    ret = SSL_write(conn->ssl, data, data_len);
    conn->last_failed_write_data_len = ret <= 0 ? data_len : 0;
    return updateStateAfterSSLIO(conn, ret, 1);
}

static int connTLSWritev(connection *conn_, const struct iovec *iov, int iovcnt) {
    tls_connection *conn = (tls_connection *)conn_;
    if (iovcnt == 1) return connTLSWrite(conn_, iov[0].iov_base, iov[0].iov_len);

    /* Accumulate the amount of bytes of each buffer and check if it exceeds NET_MAX_WRITES_PER_EVENT. */
    size_t iov_bytes_len = 0;
    for (int i = 0; i < iovcnt; i++) {
        iov_bytes_len += iov[i].iov_len;
        if (iov_bytes_len > NET_MAX_WRITES_PER_EVENT) break;
    }

    /* In case the amount of all buffers is greater than NET_MAX_WRITES_PER_EVENT,
     * it might not worth doing so much memory copying to reduce system calls,
     * therefore, invoke connTLSWrite() multiple times to avoid memory copies.
     * However, in case when last write failed we still have to repeat sending last_failed_write_data_len