Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

panic on non nil "Result" with nil value in "AfterQuery" hook #1942

Open
server-may-cry opened this issue Jan 14, 2022 · 0 comments
Open

panic on non nil "Result" with nil value in "AfterQuery" hook #1942

server-may-cry opened this issue Jan 14, 2022 · 0 comments

Comments

@server-may-cry
Copy link

Query hook receives non nil Result with nil value. This leads to a panic:

runtime error: invalid memory address or nil pointer dereference

Expected Behavior

Code like this don't trigger a nil pointer panic as it have a e.Result != nil check.

func (h hook) AfterQuery(ctx context.Context, e *pg.QueryEvent) error {
	if e.Err != nil {
		h.log("error", e.Err)
	}
	if e.Result != nil {
		h.log("returned", e.Result.RowsReturned())
		h.log("affected", e.Result.RowsAffected())
	}
	return nil
}

Current Behavior

Part of panic stack trace

	/usr/local/go/src/runtime/panic.go:1038 +0x215
github.com/go-pg/pg/v10.(*result).RowsReturned(0xdc4c80)
	/go/pkg/mod/github.com/go-pg/pg/[email protected]/result.go:52
<REDACTED>.AfterQuery({{0xdae1c0, 0xc00011e110}}, {0xdc4c80, 0xc000b5b7d0}, 0xc0004a1900)
	<REDACTED>.go:110 +0xf2
github.com/go-pg/pg/v10.(*baseDB).afterQueryFromIndex(0xc0000af0e0, {0xdc4c80, 0xc000b5b7d0}, 0x0, 0xdd8890)
	/go/pkg/mod/github.com/go-pg/pg/[email protected]/hook.go:130 +0x75
github.com/go-pg/pg/v10.(*baseDB).afterQuery(0xc000146d00, {0xdc4c80, 0xc000b5b7d0}, 0xc0009c8005, {0xdbce20, 0x0}, {0xdae1c0, 0xc00011e110})
	/go/pkg/mod/github.com/go-pg/pg/[email protected]/hook.go:125 +0xaa

Possible Solution

Explicitly set nil for Result when *result is nil.
Related article https://codefibershq.com/blog/golang-why-nil-is-not-always-nil

Steps to Reproduce

  1. Add a query hook as provided above
  2. Execute SQL which fails
  3. nil pointer panic is triggered

Here is an example of how nil becomes not nil https://goplay.tools/snippet/DVV1XNz5icn

Context (Environment)

Detailed Description

On first line res is a nil. At the last line it assigned a value from db.simpleQuery

pg/base.go

Lines 236 to 246 in 51b8018

var res Result
var lastErr error
for attempt := 0; attempt <= db.opt.MaxRetries; attempt++ {
if attempt > 0 {
if err := internal.Sleep(ctx, db.retryBackoff(attempt-1)); err != nil {
return nil, err
}
}
lastErr = db.withConn(ctx, func(ctx context.Context, cn *pool.Conn) error {
res, err = db.simpleQuery(ctx, cn, wb)

In db.simpleQuery result might get returned with nil value of type *result, but in db.simpleQuery it will be wrapped into Result and will pass != nil check

pg/base.go

Lines 544 to 553 in 51b8018

var res *result
if err := cn.WithReader(c, db.opt.ReadTimeout, func(rd *pool.ReaderContext) error {
var err error
res, err = readSimpleQuery(rd)
return err
}); err != nil {
return nil, err
}
return res, nil

Note: Same issue exists in other parts of a code.

Possible Implementation

Workaround of this issue is to change a hook to look like this:

func (h hook) AfterQuery(ctx context.Context, e *pg.QueryEvent) error {
	if e.Err != nil {
		h.log("error", e.Err)
	} else if e.Result != nil {
		h.log("returned", e.Result.RowsReturned())
		h.log("affected", e.Result.RowsAffected())
	}
	return nil
}
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

No branches or pull requests

1 participant