Skip to content

Add fuzzer: FuzzEncodeFromJSON which signals roundtripping issues #35

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add fuzzer: FuzzEncodeFromJSON which signals roundtripping issues #35

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
//go:build go1.18
// +build go1.18
Comment on lines +1 to +2
Copy link
Member

@stefanprodan stefanprodan Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove the go version from here

Copy link
Author

@dolmen dolmen Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we now have "go 1.18" in go.mod this build guard is redundant.
So 👍


package yaml_test

import (
"bytes"
"encoding/json"
"testing"

"go.yaml.in/yaml/v3"
Copy link
Member

@stefanprodan stefanprodan Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"go.yaml.in/yaml/v3"
"go.yaml.in/yaml/v4"

)

// FuzzEncodeFromJSON checks that any JSON encoded value can also be encoded as YAML... and decoded.
func FuzzEncodeFromJSON(f *testing.F) {
f.Add(`null`)
f.Add(`""`)
f.Add(`0`)
f.Add(`true`)
f.Add(`false`)
f.Add(`{}`)
f.Add(`[]`)
f.Add(`[[]]`)
f.Add(`{"a":[]}`)
Copy link
Contributor

@ccoVeille ccoVeille Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this also

Suggested change
f.Add(`{"a":[]}`)
f.Add(`{"a":{}}`)
f.Add(`{"a":[]}`)

f.Add(`-0`)
f.Add(`-0.000`)
f.Add(`"\n"`)
f.Add(`"\t"`)

f.Fuzz(func(t *testing.T, s string) {

var v interface{}
if err := json.Unmarshal([]byte(s), &v); err != nil {
t.Skip("not valid JSON")
}

t.Logf("JSON %q", s)
Comment on lines +34 to +37
Copy link
Contributor

@ccoVeille ccoVeille Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maybe

Suggested change
t.Skip("not valid JSON")
}
t.Logf("JSON %q", s)
t.Skipf("not valid JSON %q", s)
}
t.Logf("JSON %q", s)

t.Logf("Go %q <%[1]x>", v)

// Encode as YAML
b, err := yaml.Marshal(v)
if err != nil {
t.Error(err)
}
t.Logf("YAML %q <%[1]x>", b)

// Decode as YAML
var v2 interface{}
if err := yaml.Unmarshal(b, &v2); err != nil {
t.Error(err)
}

t.Logf("Go %q <%[1]x>", v2)

/*
// Handling of number is different, so we can't have universal exact matching
if !reflect.DeepEqual(v2, v) {
t.Errorf("mismatch:\n- got: %#v\n- expected: %#v", v2, v)
}
*/
Comment on lines +55 to +60
Copy link
Contributor

@ccoVeille ccoVeille Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always suspicious when I see commented out code.

What is the need here?

Can't it be simply removed? Is it a left behind debug or something that was planned but that was abandoned?


b2, err := yaml.Marshal(v2)
if err != nil {
t.Error(err)
}
t.Logf("YAML %q <%[1]x>", b2)

if !bytes.Equal(b, b2) {
t.Errorf("Marshal->Unmarshal->Marshal mismatch:\n- expected: %q\n- got: %q", b, b2)
Copy link
Member

@ingydotnet ingydotnet Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dolmen , what do you think about changing this to t.Logf, so that we can merge the PR to main and always see where the problems are?

If we error, we can't really merge this.

Copy link
Member

@ingydotnet ingydotnet Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it stops on the first error. Changing to Logf lets us see all the issues and fix them one at a time (and add a regression test for each fix).

Copy link
Contributor

@carloslima carloslima Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #45 to address the -0 case failure in the description of this issue.

With that fix I was able to run a 1m fuzzing test successfully:

> go test -fuzz FuzzEncodeFromJSON -fuzztime=1m

OK: 50 passed
fuzz: elapsed: 0s, gathering baseline coverage: 0/9 completed
fuzz: elapsed: 0s, gathering baseline coverage: 9/9 completed, now fuzzing with 20 workers
fuzz: elapsed: 3s, execs: 313723 (104563/sec), new interesting: 316 (total: 325)
fuzz: elapsed: 6s, execs: 526587 (70945/sec), new interesting: 411 (total: 420)
fuzz: elapsed: 9s, execs: 576090 (16504/sec), new interesting: 421 (total: 430)
fuzz: elapsed: 12s, execs: 584466 (2792/sec), new interesting: 426 (total: 435)
fuzz: elapsed: 15s, execs: 589110 (1548/sec), new interesting: 433 (total: 442)
fuzz: elapsed: 18s, execs: 592742 (1211/sec), new interesting: 438 (total: 447)
fuzz: elapsed: 21s, execs: 595062 (773/sec), new interesting: 441 (total: 450)
fuzz: elapsed: 24s, execs: 601436 (2124/sec), new interesting: 443 (total: 452)
fuzz: elapsed: 27s, execs: 604790 (1118/sec), new interesting: 449 (total: 458)
fuzz: elapsed: 30s, execs: 606250 (487/sec), new interesting: 450 (total: 459)
fuzz: elapsed: 33s, execs: 609672 (1141/sec), new interesting: 451 (total: 460)
fuzz: elapsed: 36s, execs: 612397 (908/sec), new interesting: 453 (total: 462)
fuzz: elapsed: 39s, execs: 613778 (460/sec), new interesting: 455 (total: 464)
fuzz: elapsed: 42s, execs: 616508 (910/sec), new interesting: 456 (total: 465)
fuzz: elapsed: 45s, execs: 619459 (984/sec), new interesting: 458 (total: 467)
fuzz: elapsed: 48s, execs: 624442 (1661/sec), new interesting: 459 (total: 468)
fuzz: elapsed: 51s, execs: 628547 (1368/sec), new interesting: 459 (total: 468)
fuzz: elapsed: 54s, execs: 632180 (1212/sec), new interesting: 459 (total: 468)
fuzz: elapsed: 57s, execs: 636113 (1311/sec), new interesting: 459 (total: 468)
fuzz: elapsed: 1m0s, execs: 671082 (11613/sec), new interesting: 460 (total: 469)
fuzz: elapsed: 1m1s, execs: 671082 (0/sec), new interesting: 460 (total: 469)
PASS
ok  	go.yaml.in/yaml/v3	65.454s

Copy link
Contributor

@carloslima carloslima Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #46 to address another issue highlighted by the fuzzing test.

With both fixes applied:

~/c/go-yaml-2 (merge-neg-zero-tabs)> go test -fuzz FuzzEncodeFromJSON
OK: 50 passed
fuzz: elapsed: 0s, gathering baseline coverage: 0/1048 completed
fuzz: elapsed: 1s, gathering baseline coverage: 1048/1048 completed, now fuzzing with 20 workers
fuzz: elapsed: 3s, execs: 191976 (63988/sec), new interesting: 1 (total: 1049)
fuzz: elapsed: 6s, execs: 507713 (105131/sec), new interesting: 5 (total: 1053)
fuzz: elapsed: 9s, execs: 835477 (109372/sec), new interesting: 12 (total: 1060)
(...)
fuzz: elapsed: 1h6m12s, execs: 89709287 (22797/sec), new interesting: 327 (total: 1375)
fuzz: elapsed: 1h6m15s, execs: 89760448 (17057/sec), new interesting: 327 (total: 1375)
fuzz: elapsed: 1h6m18s, execs: 89824044 (21190/sec), new interesting: 328 (total: 1376)
^C
fuzz: elapsed: 1h6m21s, execs: 89891344 (22440/sec), new interesting: 328 (total: 1376)
fuzz: elapsed: 1h6m21s, execs: 89891344 (0/sec), new interesting: 328 (total: 1376)
PASS
ok  	go.yaml.in/yaml/v3	3985.504s

Copy link
Contributor

@carloslima carloslima Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we error, we can't really merge this.

Fuzzing only happens when you explicitly run it with go test -fuzz <testname>, otherwise they act like normal tests: https://go.dev/doc/security/fuzz/#running-fuzz-tests

Fuzz tests are run much like a unit test by default. Each seed corpus entry will be tested against the fuzz target, reporting any failures before exiting.
To enable fuzzing, run go test with the -fuzz flag, providing a regex matching a single fuzz test.

This looks fine to merge.

}

})
}
Loading