Assertion `is_name()` failed at wasm-decompiler caused by empty global name

[Mem2019]

trafficstars

Commit: d8517aa922ddd3ffd6d7f28f00d2bb0f9853cf88

Reproduce

Fuzzed Sample: wabt_decomp_is_name_assert.zip

Simplified Sample:

$ cat simplified.wat 
(module
  (type (;0;) (func (result i64)))
  (func (;0;) (type 0) (result i64)
    global.get 0)
  (global (;0;) i64 (i64.const 8))
  (export "" (global 0))
)
$ ./wat2wasm ./simplified.wat && ./wasm-decompile ./simplified.wasm 
wasm-decompile: /wabt/src/ir.h:61: const string& wabt::Var::name() const: Assertion `is_name()' failed.
Aborted (core dumped)

Analysis

struct Var has a union field, and it has a type_ field to indicate which union entry should be accessed.

// Definition of VarType
enum class VarType {
  Index,
  Name,
};

// Private fields of `struct Var` 
  VarType type_;
  union {
    Index index_;
    std::string name_;
  };

If type_ is Index, index_ should be accessed; if type_ is Name, name_ should be accessed. Current assertion error is caused by we are accessing name_ of a struct Var whose type_ is Index, and in release build in which assert is removed, the type confusion can occur (e.i. an unsigned integer will be used as a pointer).

Root Cause

Here is the intended behavior first:

1. ReadBinaryIr is called in ProgramMain of wasm-decompile to parse the binary, in which Var var field of GlobalGetExpr is initialized to type_ == Index first.
2. Then ApplyNames in ProgramMain is called to convert var field of every GlobalGetExpr to type_ == Name version.
3. Therefore when decompiling this expression at Decompiler::DecompileExpr, the assertion must hold.

However, the problem occurs when name of global is empty (e.i. == ""). In such case, type_ field will not be converted into Name version and will still be Index, which causes the assertion error.

Here is the reason why name of global becomes empty, which makes sense intuitively because export name is given as empty string at (export "" (global 0)):

1. When NameGenerator::VisitExport is called inside GenerateNames, MaybeUseAndBindName will be called with export_->name.c_str() as an empty C string. Therefore the global name is assigned to "$".
2. Then RenameToIdentifier called inside RenameAll would rename "$" into "", which makes the global name become an empty string.

Possible Fix

I think the best location to fix this bug is at RenameToIdentifier, because even if export_->name.c_str() is not empty but something like " $_?", the bug can still be triggered. The reason is that RenameToIdentifier always skips leading non-alphanumeric character and generates empty string when there is no alphanumeric character. Therefore this situation should be tackled.

[sbc100]

Thanks for the detailed analysis.

I appears that wasm-decompile expects that all Var's will have names .. but ApplyNames doesn't seem to guarantee this.

I think I agree that if we ensured that RenameToIdentifier could never return an empty string that would be good and it would ensure that ApplyNames gave a name to every single Var.

If you have time a write a patch it would be most welcome.

[sbc100]

Actually it looks like a slight modification to src/generate-names.cc does the trick:

bool NameGenerator::HasName(const std::string& str) {                            
  return !str.empty() && str != "$";                                                                   
}

if this behaviour isn't always desired for all callers of GenerateNames then we could add another option perhaps.

[Mem2019]

Actually it looks like a slight modification to src/generate-names.cc does the trick:

bool NameGenerator::HasName(const std::string& str) {                            
  return !str.empty() && str != "$";                                                                   
}

if this behaviour isn't always desired for all callers of GenerateNames then we could add another option perhaps.

No. As I have mentioned that non-alphanumeric export name like " $_?" can also trigger the bug, so checking against "$" does not solve the problem. If we want to solve the problem here, we need to consider all possible cases that might cause the identifier to be empty string, which can easily cause incomplete fix.