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

Recent false positive from dd-trace-go SpanFromContext? #199

Open
froodian opened this issue Feb 9, 2024 · 1 comment
Open

Recent false positive from dd-trace-go SpanFromContext? #199

froodian opened this issue Feb 9, 2024 · 1 comment
Labels
false positive Requires more analysis and support

Comments

@froodian
Copy link

froodian commented Feb 9, 2024

NilAway recently started complaining on some code it was happy with previously. The code looks like

span, _ := tracer.SpanFromContext(ctx)
span.Finish(tracer.WithError(err))

and the error

/path/to/my-package/file.go:43:3: error: Potential nil panic detected. Observed nil flow from source to dereference point:
	- my-package/file.go:43:3: result 0 of `SpanFromContext()` lacking guarding; called `Finish()` via the assignment(s):
		- `tracer.SpanFromContext(ctx)` to `span` at my-package/file.go:42:3

but internally SpanFromContext, implemented here, cannot return nil:

	if ctx == nil {
		return &traceinternal.NoopSpan{}, false
	}
	v := ctx.Value(internal.ActiveSpanKey)
	if s, ok := v.(ddtrace.Span); ok {
		return s, true
	}
	return &traceinternal.NoopSpan{}, false
@sonalmahajan15
Copy link
Contributor

@froodian: This error is getting reported after the merging of PR #157 . The logic in that PR includes the assumption that if the function has an ok-form, then the caller can safely access other returns only after checking ok to be true. Hence, NilAway fails fast if it finds that the ok return is suppressed. But clearly in this case the assumption is problematic, and we shouldn't fail fast. I'll investigate more and implement a solution to safeguard NilAway against such false positives. Thanks for reporting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false positive Requires more analysis and support
Projects
None yet
Development

No branches or pull requests

2 participants