arocc icon indicating copy to clipboard operation
arocc copied to clipboard

Potential crash with non-integer array size

Open ehaas opened this issue 2 years ago • 3 comments

Strange one found via fuzzing. If the array size specifier is .invalid:

[-w-f]" "f [f]" "f[-w-f]
panic: reached unreachable code
/Users/ehaas/source/arocc/src/aro/Type.zig:2345:21: 0x1080024ce in fromType (arocc)
            else => unreachable,
                    ^
/Users/ehaas/source/arocc/src/aro/Parser.zig:435:30: 0x108000b88 in typeStr (arocc)
    if (Type.Builder.fromType(ty).str(p.comp.langopts)) |str| return str;
                             ^
/Users/ehaas/source/arocc/src/aro/Parser.zig:2984:70: 0x10817ad8b in directDeclarator (arocc)
            try p.errStr(.array_size_non_int, size_tok, try p.typeStr(size.ty));
                                                                     ^
/Users/ehaas/source/arocc/src/aro/Parser.zig:2879:38: 0x1080e7d99 in declarator (arocc)
        d.ty = try p.directDeclarator(d.ty, &d, kind);
                                     ^
/Users/ehaas/source/arocc/src/aro/Parser.zig:1746:31: 0x1080e529c in initDeclarator (arocc)
        .d = (try p.declarator(decl_spec.ty, .normal)) orelse return null,
                              ^

ehaas avatar Nov 27 '23 19:11 ehaas

Fixed by something like:

diff --git a/src/aro/Parser.zig b/src/aro/Parser.zig
index ebaa318..3b84321 100644
--- a/src/aro/Parser.zig
+++ b/src/aro/Parser.zig
@@ -2980,6 +2980,7 @@ fn directDeclarator(p: *Parser, base_type: Type, d: *Declarator, kind: Declarato
         if (max_bits > 61) max_bits = 61;
         const max_bytes = (@as(u64, 1) << @truncate(max_bits)) - 1;
 
+        if (size.ty.is(.invalid)) return Type.invalid;
         if (!size.ty.isInt()) {
             try p.errStr(.array_size_non_int, size_tok, try p.typeStr(size.ty));
             return error.ParsingFailed;

Vexu avatar Nov 27 '23 21:11 Vexu

Here's an alternate path to trigger it:

struct Foo {
    int a;
} a;

char *f(char *arg) {
    return arg + a?a;
}

Do you think we should check in the typeStr functions? I'm guessing any place we call that could potentially have an invalid type

ehaas avatar Nov 28 '23 04:11 ehaas

That would solve the issue but it'd be nicer to properly handle invalid where needed. Maybe we could add a release mode check for invalid in typeStr* so that release modes don't crash?

Vexu avatar Nov 28 '23 11:11 Vexu