Skip to content

Fix bugs detected by the address sanitizer - #2212

Merged
jblueh merged 23 commits into
developfrom
fix_addresssanitizer_findings
Feb 17, 2024
Merged

Fix bugs detected by the address sanitizer#2212
jblueh merged 23 commits into
developfrom
fix_addresssanitizer_findings

Conversation

@maxaehle

@maxaehle maxaehle commented Feb 14, 2024

Copy link
Copy Markdown
Contributor

Steps to reproduce the underlying issue

Compile SU2 with Clang and -fsanitize=address (and possibly -fno-omit-frame-pointer), and run the testcases as usual. AddressSanitizer warnings appear in the output (if any).

So far, only the tests in serial_regression.py have been analyzed.

Proposed Changes

Depending on the context of the memory leak,

  • add delete / delete[] statement
  • turn new-allocated arrays into std::vectors, or
  • for one bug caused by a delete to a base class pointer, make its destructor virtual.

Related Work

#2211 and #2213 have been detected in the same way, but I'm not sure how to properly fix them.

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • [not necessary] I have added a test case that demonstrates my contribution, if necessary.
  • [not necessary] I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

Max Aehle added 6 commits February 14, 2024 14:16
In CPhysicalGeometry.cpp, instances of derived classes are
delete'd via CMeshReaderFVM pointers. Without a virtual
destructor, destructors of members added by these derived classes
are not called. With respect to e.g. the std::vector members of
CCGNSMeshReaderFVM, this leads to memory leaks.

@pcarruscag pcarruscag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice catches

Comment thread Common/include/option_structure.inl Outdated
Comment thread SU2_CFD/include/solvers/CFVMFlowSolverBase.inl Outdated
jblueh and others added 5 commits February 15, 2024 10:45
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
This helps to avoid memory leaks and double frees from the
respective COptionActDisk instances, and allows to check that
identical marker lists were supplied.
@jblueh

jblueh commented Feb 17, 2024

Copy link
Copy Markdown
Contributor

The contributions I made are from investigating serial_regression_AD.py in the same manner. parallel_regression.py and parallel_regression_AD.py are checked to a certain extent, too.

@jblueh
jblueh marked this pull request as ready for review February 17, 2024 01:27
@jblueh
jblueh merged commit 0f211b5 into develop Feb 17, 2024
@jblueh
jblueh deleted the fix_addresssanitizer_findings branch February 17, 2024 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants