Skip to content

rpcserver: nil-guard AllowHighFees in handleSendRawTransaction - #3816

Open
methakon wants to merge 1 commit into
decred:masterfrom
methakon:fix/3813-nil-allowhighfees
Open

methakon wants to merge 1 commit into
decred:masterfrom
methakon:fix/3813-nil-allowhighfees

Conversation

@methakon

Copy link
Copy Markdown

Problem

sendrawtransaction takes an optional AllowHighFees *bool with a jsonrpcdefault:"false" tag. The parser applies that declared default only when the field is absent, so a client that sends an explicit JSON null for it arrives with a nil pointer. The handler dereferenced it as its first effective statement:

allowHighFees := *c.AllowHighFees

The handler runs on a goroutine with no recover, so this is a process crash rather than a failed request. The limited tier of RPC credential is enough to trigger it, and the same request with the parameter omitted works correctly.

Why it went unnoticed: every case in the existing TestHandleSendRawTransaction constructs the command with a non-nil pointer, because that is the natural way to write the test literal. The nil state is only reachable through a parsed request, not a constructed one.

Fix

Nil-guard the dereference, matching the idiom already used throughout this command set (if c.Verbose != nil && !*c.Verbose, and the other optional pointer parameters the report counts):

allowHighFees := c.AllowHighFees != nil && *c.AllowHighFees

Absent and explicit-false then behave identically, which is what the declared default already implies: a request that does not opt in to high fees is treated as one that declines them. No signature or wire change, so clients are unaffected.

Test

One case added to the existing table, placed first so the panic is the first thing exercised. It fails on the unfixed handler:

--- FAIL: TestHandleSendRawTransaction/handleSendRawTransaction:_nil_allowHighFees
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x6da06d]
	github.com/decred/dcrd/internal/rpcserver.handleSendRawTransaction
		/src/internal/rpcserver/rpcserver.go:4403 +0x4d

and passes with the guard. The mock sync manager is set to return a processing error so the case asserts the handler reaches the deserialization path and returns the existing error code, rather than merely not panicking.

Verification

Run in a clean golang:1.25 container against master (6f6cf21):

Command Result
go test ./internal/rpcserver/ -run TestHandleSendRawTransaction SIGSEGV before, ok after
go test ./internal/rpcserver/... -count=1 ok, 5.5s, whole package
go vet ./internal/rpcserver/... clean
gofmt -l on both touched files clean

A note on the class, not just this instance

If the parser skips a declared default for an explicit null, then every optional pointer parameter in the command set carries this hazard, and the ones that are currently safe are safe only because each dereference site is guarded by hand. Twenty-odd hand-written guards is a lot of places to keep in step.

Normalising the parser so an explicit null takes the declared default would remove the class entirely. That changes parsing behaviour for every command in the set, so it is a wider decision than a crash fix and I have deliberately not folded it in here. Raising it in case it is worth doing on its own.

The parameter is optional and typed *bool, so a client may send an explicit
JSON null for it. The declared jsonrpcdefault is only applied when the field is
absent, which leaves the pointer nil, and the handler dereferenced it as its
first effective statement. The handler runs on a goroutine with no recover, so
the result is a process crash rather than a failed request, and the limited
tier of RPC credential is enough to trigger it.

Nil-guard the dereference the same way the other optional pointer parameters
in this command set are guarded at their dereference sites. Absent and explicit
false then behave identically, which is what the declared default already
implies: a request that does not opt in to high fees declines them. No signature
or wire change, so clients are unaffected.

The existing table test missed this because every case constructs the command
with a non-nil pointer, as one naturally does. The nil state is only reachable
through a parsed request. The new case is first in the table so the panic is the
first thing exercised, and it fails on the unfixed handler with a SIGSEGV at
this line before the guard is added.

Fixes decred#3813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant