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.

Sign up for GitHub

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

Jump to bottom

log/slog: JSONHandler generates invalid JSON with empty Attr #62152

Closed
jaloren opened this issue Aug 19, 2023 · 6 comments
Closed

log/slog: JSONHandler generates invalid JSON with empty Attr #62152

jaloren opened this issue Aug 19, 2023 · 6 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.22

Comments

@jaloren
Copy link

jaloren commented Aug 19, 2023 •
edited by panjf2000

What version of Go are you using (go version)?

1.21.0

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

This also reproduces on the go playground.

go env Output
GO111MODULE=''
GOARCH='arm64'
GOBIN='/Users/jlorenzini/go/bin'
GOCACHE='/Users/jlorenzini/Library/Caches/go-build'
GOENV='/Users/jlorenzini/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/jlorenzini/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/jlorenzini/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/jlorenzini/repos/foo/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/62/n3p69dvx45l3c3wgng9c01sw0000gn/T/go-build1832919248=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I executed this code in a main function. This can also be reproduced in the go playground: https://go.dev/play/p/i7VtMtNK4eV

var buf bytes.Buffer
	f := func(groups []string, a slog.Attr) slog.Attr {
		return slog.Attr{}
	}
	h := slog.NewJSONHandler(&buf, &slog.HandlerOptions{

		ReplaceAttr: f})

	results := func() []map[string]any {
		var ms []map[string]any
		for _, line := range bytes.Split(buf.Bytes(), []byte{'\n'}) {
			if len(line) == 0 {
				continue
			}
			var m map[string]any
			if err := json.Unmarshal(line, &m); err != nil {
				panic(err) // In a real test, use t.Fatal.
			}
			ms = append(ms, m)
		}
		return ms
	}
	err := slogtest.TestHandler(h, results)
	if err != nil {
		log.Fatal(err)
	}

What did you expect to see?

The program run successfully without an error.

What did you see instead?

The program panicked with the following error when json unmarshal happened.

panic: invalid character ',' looking for beginning of object key string
@dmitshur dmitshur changed the title affected/package: log/slog/JSONHandler generates invalid json with empty slog.Attr log/slog: JSONHandler generates invalid JSON with empty Attr Aug 19, 2023
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 19, 2023
@dmitshur
Copy link
Contributor

CC @jba.

@dmitshur dmitshur added this to the Backlog milestone Aug 19, 2023
@gopherbot
Copy link

gopherbot commented Aug 19, 2023 •
edited by panjf2000

Change https://go.dev/cl/521135 mentions this issue: log/slog: generate valid JSON string with empty attributes in Groups

@panjf2000
Copy link
Member

panjf2000 commented Aug 19, 2023 •
edited

This is a bug and it can occur in scenarios where users try to replace some attributes in embedded groups with empty strings. I believe these cases could be common for users.

@panjf2000 panjf2000 added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 19, 2023
@panjf2000 panjf2000 modified the milestones: Backlog, Go1.22 Aug 19, 2023
@panjf2000
Copy link
Member

@gopherbot please consider this for backport to 1.21, it could be a common case and has no workaround.

@gopherbot
Copy link

Backport issue(s) opened: #62256 (for 1.21).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Aug 24, 2023
Closed
@jba
Copy link
Contributor

jba commented Sep 7, 2023

I don't think this requires a backport. It's probably very rare, and you can work around it by just not removing every attr (or adding a dummy one, if you have to remove them all for some security reason).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
6 participants
@dmitshur @jaloren @panjf2000 @gopherbot @jba and others