Skip to content

Conversation

@zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented May 27, 2025

When creating an outgoing TLS connection, we don't check if SSL_new() returned NULL.

Without this patch, the check was done only for incoming connections in connCreateAcceptedTLS(). This patch moves the check to createTLSConnection() which is used both for incoming and outgoing connections.

This check makes sure we fail the connection before going any further, e.g. when connCreate() is followed by connConnect(), the latter returns C_ERR which is commonly detected where outgoing connections are established, such where a replica connects to a primary.

int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }

For a more thorough explanation, see #1939 (comment).

Might fix #1939.

Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

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

Make sense to me.

@zuiderkwast zuiderkwast added the release-notes This issue should get a line item in the release notes label May 27, 2025
@codecov
Copy link

codecov bot commented May 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.21%. Comparing base (3ceae81) to head (1d09511).
Report is 3 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2140      +/-   ##
============================================
- Coverage     71.23%   71.21%   -0.02%     
============================================
  Files           122      122              
  Lines         66049    66049              
============================================
- Hits          47051    47039      -12     
- Misses        18998    19010      +12     
Files with missing lines Coverage Δ
src/tls.c 100.00% <ø> (ø)

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zuiderkwast zuiderkwast merged commit 17e6686 into valkey-io:unstable May 27, 2025
51 checks passed
@github-project-automation github-project-automation bot moved this to To be backported in Valkey 8.0 May 27, 2025
@github-project-automation github-project-automation bot moved this to To be backported in Valkey 7.2 May 27, 2025
@github-project-automation github-project-automation bot moved this to To be backported in Valkey 8.1 May 27, 2025
@zuiderkwast zuiderkwast deleted the ssl-new-error branch May 27, 2025 18:31
hpatro pushed a commit to hpatro/valkey that referenced this pull request Jun 4, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
hpatro pushed a commit to hpatro/valkey that referenced this pull request Jun 4, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
hpatro pushed a commit that referenced this pull request Jun 9, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
#1939 (comment).

Might fix #1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
hpatro pushed a commit that referenced this pull request Jun 11, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
#1939 (comment).

Might fix #1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
@hpatro hpatro moved this from To be backported to 8.1.2 in Valkey 8.1 Jun 11, 2025
chzhoo pushed a commit to chzhoo/valkey that referenced this pull request Jun 12, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: chzhoo <[email protected]>
vitarb pushed a commit to vitarb/valkey that referenced this pull request Jun 12, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
(cherry picked from commit 17e6686)
@vitarb vitarb mentioned this pull request Jun 13, 2025
vitarb pushed a commit to vitarb/valkey that referenced this pull request Jun 13, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
(cherry picked from commit 17e6686)
shanwan1 pushed a commit to shanwan1/valkey that referenced this pull request Jun 13, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: shanwan1 <[email protected]>
@ranshid ranshid moved this from To be backported to In Progress in Valkey 7.2 Jun 18, 2025
ranshid pushed a commit to ranshid/valkey that referenced this pull request Jun 18, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
ranshid pushed a commit to ranshid/valkey that referenced this pull request Jun 22, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
ranshid pushed a commit that referenced this pull request Jul 7, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
#1939 (comment).

Might fix #1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
zuiderkwast added a commit to vitarb/valkey that referenced this pull request Aug 15, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
(cherry picked from commit 17e6686)
zuiderkwast added a commit to vitarb/valkey that referenced this pull request Aug 15, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
(cherry picked from commit 17e6686)
Signed-off-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast moved this from To be backported to 8.0.5 in Valkey 8.0 Aug 18, 2025
zuiderkwast added a commit to vitarb/valkey that referenced this pull request Aug 21, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
(cherry picked from commit 17e6686)
Signed-off-by: Viktor Söderqvist <[email protected]>
zuiderkwast added a commit that referenced this pull request Aug 22, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
#1939 (comment).

Might fix #1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
(cherry picked from commit 17e6686)
Signed-off-by: Viktor Söderqvist <[email protected]>
sarthakaggarwal97 pushed a commit to sarthakaggarwal97/valkey that referenced this pull request Sep 16, 2025
When creating an outgoing TLS connection, we don't check if `SSL_new()`
returned NULL.

Without this patch, the check was done only for incoming connections in
`connCreateAcceptedTLS()`. This patch moves the check to
`createTLSConnection()` which is used both for incoming and outgoing
connections.

This check makes sure we fail the connection before going any further,
e.g. when `connCreate()` is followed by `connConnect()`, the latter
returns `C_ERR` which is commonly detected where outgoing connections
are established, such where a replica connects to a primary.

```c
int connectWithPrimary(void) {
    server.repl_transfer_s = connCreate(connTypeOfReplication());
    if (connConnect(server.repl_transfer_s, server.primary_host, server.primary_port, server.bind_source_addr,
                    server.repl_mptcp, syncWithPrimary) == C_ERR) {
        serverLog(LL_WARNING, "Unable to connect to PRIMARY: %s", connGetLastError(server.repl_transfer_s));
        connClose(server.repl_transfer_s);
        server.repl_transfer_s = NULL;
        return C_ERR;
    }
```

For a more thorough explanation, see
valkey-io#1939 (comment).

Might fix valkey-io#1939.

Signed-off-by: Viktor Söderqvist <[email protected]>
(cherry picked from commit 17e6686)
Signed-off-by: Viktor Söderqvist <[email protected]>
@rainsupreme rainsupreme moved this from In Progress to 7.2.10 in Valkey 7.2 Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes This issue should get a line item in the release notes

Projects

Status: 7.2.10
Status: 8.0.5
Status: 8.1.2

Development

Successfully merging this pull request may close these issues.

[CRASH] Crash during upgrade from Redis 6.2.14 to Valkey 8.0.2

2 participants