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

fuzzgen: Fuzz Switch API #4502

Merged
merged 2 commits into from
Jul 25, 2022
Merged

fuzzgen: Fuzz Switch API #4502

merged 2 commits into from
Jul 25, 2022

Conversation

afonso360
Copy link
Contributor

Hey,

Turns out this is an interface that the frontend provides.
We should fuzz it.

cc: @jameysharp

@afonso360 afonso360 changed the title fuzzgen: Use Switch interface fuzzgen: Fuzz Switch API Jul 21, 2022
@bjorn3
Copy link
Contributor

bjorn3 commented Jul 21, 2022

As author of this api, I'm curious if it will be able to find any issues.

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Jul 21, 2022
@afonso360 afonso360 force-pushed the fuzzgen-switch branch 3 times, most recently from 4b9d54f to e23388e Compare July 21, 2022 21:29
@afonso360
Copy link
Contributor Author

afonso360 commented Jul 21, 2022 •
edited
Loading

We got a crash!

test interpret

function %a(i32, i16, i32) -> i8 system_v {
    jt0 = jump_table [block1, block1, block1, block1, block1, block1, block1, block1]
    jt1 = jump_table []

block0(v0: i32, v1: i16, v2: i32):
    v3 = iconst.i32 256
    v4 = iconst.i8 1
    v24 -> v4
    v5 = bconst.b1 false
    v6 = bconst.b1 false
    v7 = bconst.b1 false
    v8 = bconst.b1 false
    v9 = bconst.b1 false
    v10 = bconst.b1 false
    v11 = bconst.b1 false
    v12 = bconst.b1 false
    v13 = bconst.b1 false
    v14 = bconst.b1 false
    v15 = bconst.b1 false
    v16 = bconst.b1 false
    v17 = bconst.b1 false
    v18 = iconst.i64 0
    v19 = iconst.i32 0
    v20 = iconst.i16 0
    v21 = iconst.i8 0
    v22 = uextend.i32 v4
    v23 = icmp_imm eq v22, 0x4100_0000_00bf_d470
    brnz v23, block1
    jump block1

block1:
    br_icmp.i8 eq v24, v24, block1
    jump block1
}
; run: %a(0, 0, 167773184) == 1

Fails with:

unexpected trap: StepError(ValueError(InvalidInteger(TryFromIntError(()))))

This is probably because the imm field is too large for an i32

Edit: Switch Entries are:

entries: {
    4683743612477887600: block1,
}

I'll submit a PR tomorrow to fix this.

Related Issue: #3059

This was referenced Jul 22, 2022
Merged
Merged
afonso360 added a commit to afonso360/wasmtime that referenced this pull request Jul 22, 2022
@afonso360
cranelift: Fix Switch bug when emitting indexes larger than val type
f74ba71 
In bytecodealliance#4502 we discovered a bug in the switch api where it would emit
`icmp_imm`'s with types that were not able to fully represent the
destination index.

The fix for this is to extend the input type to a type suitable for
representing the largest index possible.
@afonso360 afonso360 mentioned this pull request Jul 22, 2022
Merged
afonso360 added a commit to afonso360/wasmtime that referenced this pull request Jul 22, 2022
@afonso360
cranelift: Fix Switch bug when emitting indexes larger than val type
915321d 
In bytecodealliance#4502 we discovered a bug in the switch api where it would emit
`icmp_imm`'s with types that were not able to fully represent the
destination index.

We now reject these inputs. The index val must always have a
type that is capable of addressing the entire range of inputs.
afonso360 added a commit to afonso360/wasmtime that referenced this pull request Jul 22, 2022
@afonso360
cranelift: Fix Switch bug when emitting indexes larger than val type
761fe89 
In bytecodealliance#4502 we discovered a bug in the switch api where it would emit
`icmp_imm`'s with types that were not able to fully represent the
destination index.

We now reject these inputs. The index val must always have a
type that is capable of addressing the entire range of inputs.
jameysharp pushed a commit that referenced this pull request Jul 22, 2022
@afonso360
cranelift: Fix Switch bug when emitting indexes larger than val type (#…
4720d09 
…4507)

In #4502 we discovered a bug in the switch api where it would emit
`icmp_imm`'s with types that were not able to fully represent the
destination index.

We now reject these inputs. The index val must always have a
type that is capable of addressing the entire range of inputs.
jameysharp
jameysharp approved these changes Jul 23, 2022
View reviewed changes
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

This PR has been great, finding bugs in obscure corners before it's even merged. Well done!

With #4510 merged the Switch API should now work for all widths, right? Are there any remaining bugs you know about? I figure I'll wait until Monday to merge this and see if oss-fuzz starts yelling.

@afonso360
Copy link
Contributor Author

afonso360 commented Jul 23, 2022 •
edited
Loading

With #4510 merged the Switch API should now work for all widths, right?

Yes.

Are there any remaining bugs you know about? I

We still need to fix this so that it doesn't emit the switch entries larger than the index type (the case that we made illegal with #4507). But otherwise no.

Ill push those changes and leave this fuzzing over the weekend to see if anything else comes up.

@afonso360
fuzzgen: Use Switch interface
be0edc6 
Turns out this is an interface that the frontend provides.
We should fuzz it.
@afonso360
Copy link
Contributor Author

Fuzzer found nothing new over the weekend with these new changes. Should be ready to merge! Lets see if oss-fuzz finds something else.

@jameysharp jameysharp merged commit 78d3e0b into bytecodealliance:main Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@jameysharp jameysharp jameysharp approved these changes

Assignees
No one assigned
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@afonso360 @bjorn3 @jameysharp