invalid memory address or nil pointer dereference in `*baseClient.initConn`

[olde-ducke]

Hello.

There seems to be a mistake in *baseClient.initConn().
The following code panics:

package main

import (
	"context"
	"fmt"
	"os"

	"github.com/redis/go-redis/v9"
)

func main() {
	rdb := redis.NewClient(&redis.Options{})
	defer rdb.Close()

	rdb.Options().MaintNotificationsConfig = nil

	resp := rdb.Publish(context.Background(), "test", "test")
	result, err := resp.Result()
	if err != nil {
		rdb.Close()
		fmt.Fprintln(os.Stderr, err)
		os.Exit(1)
	}

	fmt.Println(result)
}

*client.Options() clearly states that the returned value should be treated as read-only and the above panic seems to be impossible to trigger through normal means without modifying *redis.Options.

However, if we take a look at the code of *baseClient.initConn(), we can see that, on line 538, the method expects a nil MaintNotificationsConfig. But there is no such check on line 540, where the EndpointType field is accessed, causing the above panic.

Considering the check right before that, this does not appear to be intentional behavior.

This panic also leaves the read lock on optLock mutex, and the following example deadlocks:

package main

import (
	"context"
	"fmt"
	"os"

	"github.com/redis/go-redis/v9"
	"github.com/redis/go-redis/v9/maintnotifications"
)

func main() {
	rdb := redis.NewClient(&redis.Options{})
	defer rdb.Close()

	rdb.Options().MaintNotificationsConfig = nil

	defer func() {
		if r := recover(); r != nil {
			rdb.Options().MaintNotificationsConfig = maintnotifications.DefaultConfig()
			_ = rdb.Publish(context.Background(), "test", "test")
		}
	}()

	resp := rdb.Publish(context.Background(), "test", "test")
	result, err := resp.Result()
	if err != nil {
		rdb.Close()
		fmt.Fprintln(os.Stderr, err)
		os.Exit(1)
	}

	fmt.Println(result)
}

Expected Behavior

0

or whatever the actual number of subscribers there was.

Current Behavior

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x5f9b91]

goroutine 1 [running]:
github.com/redis/go-redis/v9.(*baseClient).initConn(0xc0000000e0, {0x785860, 0x980840}, 0xc000112000)
	github.com/redis/go-redis/v9@v9.17.2/redis.go:540 +0xd51
github.com/redis/go-redis/v9.(*baseClient)._getConn(0xc0000000e0, {0x785860, 0x980840})
	github.com/redis/go-redis/v9@v9.17.2/redis.go:293 +0x74
github.com/redis/go-redis/v9.(*baseClient).getConn(0xc0000000e0, {0x785860?, 0x980840?})
	github.com/redis/go-redis/v9@v9.17.2/redis.go:272 +0x65
github.com/redis/go-redis/v9.(*baseClient).withConn(0xc0000000e0, {0x785860, 0x980840}, 0xc0000f3d70)
	github.com/redis/go-redis/v9@v9.17.2/redis.go:645 +0x49
github.com/redis/go-redis/v9.(*baseClient)._process(0x5fb94d?, {0x785860?, 0x980840?}, {0x787618?, 0xc00007c240?}, 0xc0000c0000?)
	github.com/redis/go-redis/v9@v9.17.2/redis.go:700 +0xe7
github.com/redis/go-redis/v9.(*baseClient).process(0xc0000000e0, {0x785860, 0x980840}, {0x787618, 0xc00007c240})
	github.com/redis/go-redis/v9@v9.17.2/redis.go:669 +0x76
github.com/redis/go-redis/v9.(*hooksMixin).processHook(...)
	github.com/redis/go-redis/v9@v9.17.2/redis.go:200
github.com/redis/go-redis/v9.(*Client).Process(0xc000065e90?, {0x785860?, 0x980840?}, {0x787618, 0xc00007c240})
	github.com/redis/go-redis/v9@v9.17.2/redis.go:1120 +0x3e
github.com/redis/go-redis/v9.cmdable.Publish(...)
	github.com/redis/go-redis/v9@v9.17.2/pubsub_commands.go:18
main.main()
	example.com/redis-bug/main.go:17 +0x153
exit status 2

or a deadlock in the second example.

Possible Solution

Probably something like:

diff --git a/redis.go b/redis.go
index a6a71067..9cdf988d 100644
--- a/redis.go
+++ b/redis.go
@@ -535,9 +535,13 @@ func (c *baseClient) initConn(ctx context.Context, cn *pool.Conn) error {
 
        // Enable maintnotifications if maintnotifications are configured
        c.optLock.RLock()
-       maintNotifEnabled := c.opt.MaintNotificationsConfig != nil && c.opt.MaintNotificationsConfig.Mode != maintnotifications.ModeDisabled
+       maintNotifEnabled := false
        protocol := c.opt.Protocol
-       endpointType := c.opt.MaintNotificationsConfig.EndpointType
+       var endpointType maintnotifications.EndpointType
+       if c.opt.MaintNotificationsConfig != nil {
+               maintNotifEnabled = c.opt.MaintNotificationsConfig.Mode != maintnotifications.ModeDisabled
+               endpointType = c.opt.MaintNotificationsConfig.EndpointType
+       }
        c.optLock.RUnlock()
        var maintNotifHandshakeErr error
        if maintNotifEnabled && protocol == 3 {

Context

I don't think this is a big problem or anything serious since the observed behavior can currently only be achieved by modifying a read-only object.

But if the library's behavior changes in the future, this could cause a real problem.

Environment

Fedora Linux 43
amd64
Redis 8.4.0
go-redis v9.17.2
go1.25.5

[ndyakov]

hello @olde-ducke, this is an interesting find and as you stated Options should be a readonly object. Do you think in the PR, we can also make it so it returns a copy of the options, so we don't have to cover every potential change of the Options that can alter the clients behaviour?

[olde-ducke]

At first, I was surprised that I could affect client behavior like that, but then I thought that the comment must have meant that returned options should be treated as read-only, meaning that if someone actually modifies them and something breaks, it's their own fault.
However, if that is the case, the comment should probably be more specific about that.

About copying the value: there doesn't seem to be a good reason to even modify options in this way, so copying them seems reasonable. But users often find creative ways to utilize this kind of an unintentional(?) behavior, which might break something for them.

ClusterClient and Ring client also expose their options in the same way.

And redisotel packages calls Options method in reports, so this change may affect its performance, though not sure by how much.

I think this change should be relatively easy to implement, but not sure what to do with potential problems mentioned above.

[ndyakov]

Hm, I am wondering if redisotel really needs the options and if it won't be safe enough to keep a read only copy of the options, which means the first time we are going to do a clone and store it as a read only options, but afterwards we are just going to return that clone?

p.s. I will mark this as maintenance since it is not a standard use of Options and not sure if we should consider it a bug.

[olde-ducke]

Hey, @ndyakov!

Sorry for the delay. Didn't have time to work on this last week.

I pushed my changes to the branch.

Surprisingly, creating a copy of an internal options field caused some tests to fail. These tests relied on the ability to modify the internal state of the client options. I could not fix some of the tests (timeout ones) without using reflect and unsafe.
So I'm not sure what to do with them.

Regarding redisotel, I was mistaken. It does call the client options method, but only during metrics initialization, so it should be fine.
I tried to reduce the number of options copies during initialization, but in the end only managed to reduce them by one.

[ndyakov]

@olde-ducke I think the test were green, so it should be fine. Left a comment in the PR, I do think we should just strongly advice against changing or using the retuned Options or treating it in any other than read-only way and this should be enough.