C++: segfault via unitialised variable when failing to read a switch-on'd struct

[johnbeard]

With a .ksy like this:

meta:
  id: uninit_issue
  title: Reproducing case for unintialized variable bug
  endian: le

seq:
  - id: item_type
    type: u4
  - id: items
    type:
      switch-on: item_type
      cases:
        0x01: substruct_01

types:
  substruct_01:
    seq:
      - id: something
        type: u4
        repeat: expr
        repeat-expr: 1024

There is a segfault reading a file that fails during parsing of substruct_01 (in this case, the stream is too short):

01 00 00 00 10 11 12 13

This is because the substruct is (maybe) created here:

uninit_issue_t::uninit_issue_t(kaitai::kstream* p__io, kaitai::kstruct* p__parent, uninit_issue_t* p__root) : kaitai::kstruct(p__io) {
    m__parent = p__parent;
    m__root = this;  // <--- (0) m_items and n_items both not init'ed in the ctor
   
    try {
        _read();
    } catch(...) {
        _clean_up();
        throw;
    }
}

void uninit_issue_t::_read() {
    m_item_type = m__io->read_u4le();
    n_items = true;
    switch (item_type()) {
    case 1: {
        n_items = false;
        m_items = new substruct_01_t(m__io, this, m__root); // <--- (1) throws on a short stream, m_items not init'ed
        break;
    }
    }
}

and then cleaned up here:

void uninit_issue_t::_clean_up() {
    if (!n_items) { // <---- (2) n_items is false, pass
        if (m_items) { // <---- (3) m_items is uninitialized
            delete m_items; m_items = 0; // <--- (4) bang
        }
    }
}

This will segfault when unlucky or when you have asan on.

Adding m_items = 0; to the ctor fixes it.

Please find attached a reproducing case:

• Build uninit_issue target
• Run uninit_issue/uninit_issue ../uninit_issue/uninit_issue_crasher.bin

archive.zip

Confirmed still an issue with the master branch compiler.

[generalmimon]

This looks quite similar to #919, but subtly different.

In both cases, I think it makes sense to adopt #245. Although it doesn't solve the problem (we still have to initialize the pointers in the constructor), having a special nullability flag n_* is unnecessary (a pointer can simply have the value of NULL, which is universally recognized) and only seems to make things more complex than they need to be. Right now, the generated C++98 code relies on the n_* flags being set correctly, which in this issue or the other issue #919 is not the case. It would be much easier to just initialize the pointer to NULL in the constructor and forget about it (and it's in fact safe to pass a null pointer to delete - "If ptr is null, does nothing.", so the if statements in _clean_up() are not necessary either).