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 issue with Cell::get_contained_cells() utility function #2873

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

jtramm
Copy link
Contributor

@jtramm jtramm commented Feb 12, 2024

Description

This PR fixes an issue in the Cell::find_parent_cells and Cell::Cell::get_contained_cells utility functions. As it is now, the functions I think are only working when distrib materials are used. However, if used on regular cells, then there is an error mode that pops up. In particular, they pop up in the ParentCellStack::compute_instance(int32_t distribcell_index) function, which looks like:

 //! compute an instance for the provided distribcell index
  int32_t compute_instance(int32_t distribcell_index) const
  {
    int32_t instance = 0;
    for (const auto& parent_cell : this->parent_cells_) {
      auto& cell = model::cells[parent_cell.cell_index];
      if (cell->type_ == Fill::UNIVERSE) {
        instance += cell->offset_[distribcell_index];
      } else if (cell->type_ == Fill::LATTICE) {
        auto& lattice = model::lattices[cell->fill_];
        instance +=
          lattice->offset(distribcell_index, parent_cell.lattice_index);
      }
    }
    return instance;
  }

As the inputted distribcell_index will be -1 when inputted for non-distribcell cell, this will lead to reading at the -1 index of the offset_ array, leading to undefined behavior.

I'm not sure how helpful it would be to develop a standalone regression test for this issue, as it deals with undefined memory access so may not expose the issue. I can confirm though that on a feature branch I'm working on for random ray the fix works (as the new feature calls the Cell::Cell::get_contained_cells function and only works correctly when the fix is in place). Let me know though @pshriwise if you'd like to see a test go in for this and I can cook one up.

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@jtramm jtramm added the Bugs label Feb 12, 2024
@jtramm jtramm requested a review from pshriwise February 12, 2024 21:08
Copy link
Contributor

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Thanks for catching this @jtramm! I'm going to make an issue on adding a regression test here. It would be appropriate to do in the C++ test suite, but this would require some programmatically generated geometry or loading an agreed-upon model that would cover a variety of testing needs for the C++ API, which is a separate issue.

@paulromano paulromano merged commit 33c910d into openmc-dev:develop Feb 15, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants