pkg/ifuzz/x86: potential issues with instruction decoding

[ramosian-glider]

pkg/ifuzz/x86 does not fully implement instruction decoding, ignoring some of the opcode field combinations. Because of that, there can be ambiguity in instruction parsing, that depends on the order of the instructions in insnset. Such problems can be triggered by shuffling the instructions at registration time:

diff --git a/pkg/ifuzz/ifuzz_test.go b/pkg/ifuzz/ifuzz_test.go
index cd87b7355..2d1526a2c 100644
--- a/pkg/ifuzz/ifuzz_test.go
+++ b/pkg/ifuzz/ifuzz_test.go
@@ -4,6 +4,7 @@
 package ifuzz
 
 import (
+       "fmt"
        "encoding/hex"
        "math/rand"
        "testing"
@@ -128,7 +129,7 @@ func testGenerate(t *testing.T, arch string) {
        insnset := iset.Arches[arch]
        r := rand.New(testutil.RandSource(t))
        for mode := iset.Mode(0); mode < iset.ModeLast; mode++ {
-               for repeat := 1; repeat < 10; repeat++ {
+               for repeat := 1; repeat < 100; repeat++ {
                        if len(insnset.GetInsns(mode, iset.TypeUser)) == 0 {
                                continue
                        }
@@ -140,10 +141,12 @@ func testGenerate(t *testing.T, arch string) {
                                Len:  repeat,
                        }
                        text := Generate(cfg, r)
+                       otext := text
                        for len(text) != 0 {
                                size, err := insnset.Decode(mode, text)
                                if size == 0 || err != nil {
-                                       t.Errorf("failed to decode text: % x", text)
+                                       t.Errorf("failed to decode text: % x", otext)
+                                       fmt.Printf("otext: % x\nremaining text: % x\n", otext, text)
                                        break
                                }
                                text = text[size:]
diff --git a/pkg/ifuzz/x86/x86.go b/pkg/ifuzz/x86/x86.go
index a2825b9e5..036fbeb48 100644
--- a/pkg/ifuzz/x86/x86.go
+++ b/pkg/ifuzz/x86/x86.go
@@ -56,6 +56,9 @@ func Register(insns []*Insn) {
        if len(insns) == 0 {
                panic("no instructions")
        }
+       rand.Shuffle(len(insns), func(i, j int) {
+               insns[i], insns[j] = insns[j], insns[i]
+       })
        insnset := &InsnSet{
                Insns: append(insns, pseudo...),
        }

[dvyukov]

If you introduce randomness into tests, please use this thing: https://github.com/google/syzkaller/blob/master/pkg/testutil/testutil.go#L25-L35

It provides both determinism for CI coverage/failures, random seeds in local testing and ability to reproduce with particular seed.

[ramosian-glider]

If you introduce randomness into tests, please use this thing: https://github.com/google/syzkaller/blob/master/pkg/testutil/testutil.go#L25-L35

It provides both determinism for CI coverage/failures, random seeds in local testing and ability to reproduce with particular seed.

Yeah, I looked into it, but it is a bit tricky to inject a random source into Register(), which constructs the InsnSet.

Perhaps I should add an (insnset *InsnSet) AddRandSource() method that will be called in the test, and that random source will be used by the x86 implementation of Decode()?

[dvyukov]

Some test-only hook that will permute instructions looks better. Note that Go tests generally run in parallel, so permuting in each test won't work well. But we could permute in an init function in pkg/ifuzz tests.