hashicorp / hashicorp/consul

Bug: Watch functions have risk of panic.

Open
#19,020 3 comments 3 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
1d 18h
Merged PRs (30d)
39

Description

#### Overview of the Issue
The following code has the risk of causing a panic as nil pointer dereference.
https://github.com/hashicorp/consul/blob/e6a111af1ad866b78f8e6e4a94eb1c901aeaf83f/api/watch/funcs.go#L182

#### Reproduction Steps
I would take us to this issue when the `plan` running `Stop` and `RunWithClientAndHclog` concurrently

### Consul info for Client
Client version: 1.8.0

### Stack Trace
Our service is using a consul client, but it is random panics while running online. These panics seem to occur approximately once per month.
![image](https://github.com/hashicorp/consul/assets/103412360/2685e714-c3ed-498d-b3de-f272803f2c7b)

```
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x9330a8]

goroutine 61437304 [running]:
github.com/hashicorp/consul/api/watch.serviceWatch.func1(0xc00466e0e0, 0x0, 0x0, 0x0, 0x0, 0xf23c80, 0xc02fab9350)
/home/work/buildspace/abd26875e7/goBuild/pkg/mod/github.com/hashicorp/consul/api@v1.8.0/watch/funcs.go:161 +0x1e8
github.com/hashicorp/consul/api/watch.(*Plan).RunWithClientAndHclog(0xc00466e0e0, 0xc0001805a0, 0x0, 0x0, 0xf3ec68, 0xc0006b0680)
/home/work/buildspace/abd26875e7/goBuild/pkg/mod/github.com/hashicorp/consul/api@v1.8.0/watch/plan.go:74 +0xe5
(0xc0392af6b0)
/home/work/buildspace/abd26875e7/watcher/consul.go:45 +0x37a
created by
/home/work/buildspace/abd26875e7/watcher/consul.go:78 +0x255
```
I followed the code by the stack info, found the following code:
https://github.com/hashicorp/consul/blame/release/1.8.0/api/watch/funcs.go#L161-L162
It's definitely not the correct location, so I disassembled the binary from our server and checkout the code address according to the pc register with the value `0x9330a8`.

### Disassembly
```
// if err != nil
0x93306d 48837c247000 CMPQ $0x0, 0x70(SP)
0x933073 7448 JE 0x9330bd
0x933075 0f57c0 XORPS X0, X0

0x933078 0f118424d8010000 MOVUPS X0, 0x1d8(SP)
0x933080 0f118424e8010000 MOVUPS X0, 0x1e8(SP)
0x933088 48898424f8010000 MOVQ AX, 0x1f8(SP)

0x933090 48898c2400020000 MOVQ CX, 0x200(SP)
0x933098 c68424bf00000000 MOVB $0x0, 0xbf(SP)
// deref p.cancelFunc
0x9330a0 488b9424b8010000 MOVQ 0x1b8(SP), DX
0x9330a8 488b02 MOVQ 0(DX), AX //////////// panic here
// call p.cancelFunc()
0x9330ab ffd0 CALL AX
0x9330ad 488bac24c0010000 MOVQ 0x1c0(SP), BP
0x9330b5 4881c4c8010000 ADDQ $0x1c8, SP
0x9330bc c3 RET
```

The above code tells me that the function pointer `p.cancelFunc` has a nil value.
The defer function `p.cancelFunc` is inlined in the caller function, so I think this is the reason why stack trace gave me the wrong info.

### Reason Analysis

1. `p.cancelFunc` was set value by `makeQueryOptionsWithContext`
https://github.com/hashicorp/consul/blob/main/api/watch/funcs.go#L178
2. `p.makeQueryOptionsWithContext` uses `p.setCancelFunc(cancel)` to set a context cancel function
https://github.com/hashicorp/consul/blob/main/api/watch/funcs.go#L340-L351
3. `setCancelFunc` return directly without setting the `p.cancelFunc`, so it is still nil
https://github.com/hashicorp/consul/blob/main/api/watch/plan.go#L242
4. When `serviceWatch` returns, the defer function is called, the function pointer dereferences and results in the panic.
https://github.com/hashicorp/consul/blob/e6a111af1ad866b78f8e6e4a94eb1c901aeaf83f/api/watch/funcs.go#L182

As our project frequently creates, starts, and stops plans, there is a risk of concurrent starting or stopping due to some poor design in our code. To mitigate this risk, we should implement a watch to ensure that the plan has fully stopped before proceeding. Please address this issue as soon as possible.

Contributor guide

Open the contributing guide

Research direction

Start with api/watch/funcs.go at the serviceWatch defer and makeQueryOptionsWithContext, then read api/watch/plan.go around setCancelFunc. Reproduce concurrent Plan Stop and RunWithClientAndHclog calls and trace the cancellation lifecycle. Done means the concurrent path no longer risks a nil-pointer panic and the plan can be confirmed fully stopped.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.