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.

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

Fix problems with ownership in validation code #301

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Apr 14, 2024

The actual fix contained in the first commit. The next commits just a minor style nits, which makes generated code looks better and gives more meaningful name to couple of constants.

The problem with validation code can be demonstrated with the following addition to debug_array_user.ksy:

# --debug (or actually --no-auto-read) with arrays of user types requires
# special handling to avoid spoiling whole object due to exception handler.
meta:
  id: debug_array_user
  ks-debug: true
seq:
  - id: one_cat
    type: cat
    valid:
      expr: _._sizeof == 1
  - id: array_of_cats
    type: cat
    repeat: expr
    repeat-expr: 3
  - id: foo
    type: u1
    valid: # should pass
      expr: _ == 1
  - id: bar
    type: s2le
    valid: # there's actually -190 in the file
      expr: _ < -190 or _ > -190
types:
  cat:
    seq:
      - id: meow
        type: u1

I just copy some checks from the valid_expr_fail.ksy test to have them in one file so it easy to check what's changed while working on another issue. I noticed, that check for one_cat generates incorrect code for C++11: one_cat() getter returns cat_t*, but the result is stored in std::unique_ptr<cat_t>. This

  • will not compile, because constructor is explicit
  • even if compiled will lead to double-free problem

The incorrect code that was generated before the fix:

void debug_array_user_t::_read() {
    m_one_cat = std::unique_ptr<cat_t>(new cat_t(m__io, this, m__root));
    m_one_cat->_read();
    {
        std::unique_ptr<cat_t> _ = one_cat();
        if (!(1 == 1)) {
            throw kaitai::validation_expr_error<std::unique_ptr<debug_array_user_t::cat_t>>(one_cat(), _io(), std::string("/seq/0"));
        }
    }
    m_array_of_cats = std::unique_ptr<std::vector<std::unique_ptr<cat_t>>>(new std::vector<std::unique_ptr<cat_t>>());
    const int l_array_of_cats = 3;
    for (int i = 0; i < l_array_of_cats; i++) {
        std::unique_ptr<cat_t> _t_array_of_cats = std::unique_ptr<cat_t>(new cat_t(m__io, this, m__root));
        _t_array_of_cats->_read();
        m_array_of_cats->push_back(std::move(_t_array_of_cats));
    }
    m_foo = m__io->read_u1();
    {
        uint8_t _ = foo();
        if (!(_ == 1)) {
            throw kaitai::validation_expr_error<uint8_t>(foo(), _io(), std::string("/seq/2"));
        }
    }
    m_bar = m__io->read_s2le();
    {
        int16_t _ = bar();
        if (!( ((_ < -190) || (_ > -190)) )) {
            throw kaitai::validation_expr_error<int16_t>(bar(), _io(), std::string("/seq/3"));
        }
    }
}

@Mingun
Copy link
Contributor Author

Mingun commented Apr 14, 2024

Also, validation_expr_error exception in C++ is dangerous because it store reference to the stack variable _ (and temporary variable before the fix), but this is another problem. Other exceptions also store references and I think they reference to the stack variables as well.

I filled kaitai-io/kaitai_struct_cpp_stl_runtime#71 for that problem.

@Mingun Mingun force-pushed the fix-validation-ownership-problems branch 2 times, most recently from 46c3ea0 to a216e8e Compare July 16, 2024 14:10
Using this KSY:
```
  - id: one_cat
    type: cat
    valid:
      expr: _._sizeof == 1
```

For C++11 changes this incorrect code:
```
m_one_cat = std::unique_ptr<cat_t>(new cat_t(m__io, this, m__root));
m_one_cat->_read();
{
    // Definition: cat_t* one_cat() { return m_one_cat.get(); }
    std::unique_ptr<cat_t> _ = one_cat();
    if (!(1 == 1)) {
        throw kaitai::validation_expr_error<std::unique_ptr<debug_array_user_t::cat_t>>(one_cat(), _io(), std::string("/seq/0"));
    }
}
```

to this:

```
m_one_cat = std::unique_ptr<cat_t>(new cat_t(m__io, this, m__root));
m_one_cat->_read();
{
    // Definition: cat_t* one_cat() { return m_one_cat.get(); }
    cat_t* _ = one_cat();
    if (!(1 == 1)) {
        throw kaitai::validation_expr_error<debug_array_user_t::cat_t*>(one_cat(), _io(), std::string("/seq/0"));
    }
}
```
The definition of variables does not use them, so here it is also unnecessary.

For C++11 changes this code:
```
m_one_cat = std::unique_ptr<cat_t>(new cat_t(m__io, this, m__root));
m_one_cat->_read();
{
    // Definition: cat_t* one_cat() { return m_one_cat.get(); }
    std::unique_ptr<cat_t> _ = one_cat();
    if (!(1 == 1)) {
        throw kaitai::validation_expr_error<std::unique_ptr<debug_array_user_t::cat_t>>(one_cat(), _io(), std::string("/seq/0"));
    }
}
```

to this:

```
m_one_cat = std::unique_ptr<cat_t>(new cat_t(m__io, this, m__root));
m_one_cat->_read();
{
    // Definition: cat_t* one_cat() { return m_one_cat.get(); }
    std::unique_ptr<cat_t> _ = one_cat();
    if (!(1 == 1)) {
        throw kaitai::validation_expr_error<std::unique_ptr<cat_t>>(one_cat(), _io(), std::string("/seq/0"));
    }
}
```
The new names better reflects purpose and that fact, that variable `_` (ITERATOR)
used not only as reference to the last parsed object in loops, but also as current
object in other contexts
@Mingun Mingun force-pushed the fix-validation-ownership-problems branch from a216e8e to 8026573 Compare October 4, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant