Skip to content

DRAFT: MCOL-6432 Fix ASan-detected memory leaks on the ColumnStore DDL path#4031

Draft
mariadb-VasilyKozhukhovskiy wants to merge 8 commits into
stable-23.10from
MCOL-6432-fix-memory-leaks-in-DDL-operations
Draft

DRAFT: MCOL-6432 Fix ASan-detected memory leaks on the ColumnStore DDL path#4031
mariadb-VasilyKozhukhovskiy wants to merge 8 commits into
stable-23.10from
MCOL-6432-fix-memory-leaks-in-DDL-operations

Conversation

@mariadb-VasilyKozhukhovskiy

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates dbcon/mysql/ha_mcs_ddl.cpp to use modern C++ memory management. Specifically, it replaces a raw pointer rowgroup::RowGroup* with a std::unique_ptr<rowgroup::RowGroup> and utilizes std::make_unique for instantiation, ensuring safer memory handling. There are no review comments, and we have no additional feedback to provide.

@mariadb-VasilyKozhukhovskiy mariadb-VasilyKozhukhovskiy force-pushed the MCOL-6432-fix-memory-leaks-in-DDL-operations branch from b0b6419 to e2061fd Compare June 16, 2026 14:56
@mariadb-VasilyKozhukhovskiy mariadb-VasilyKozhukhovskiy force-pushed the MCOL-6432-fix-memory-leaks-in-DDL-operations branch from e2061fd to a599432 Compare June 16, 2026 14:57
@mariadb-VasilyKozhukhovskiy

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request improves memory management by cleaning up allocated members in the AtaRenameColumn destructor and replacing a raw pointer with std::unique_ptr in ha_mcs_ddl.cpp. The review feedback suggests utilizing a range-based for loop in the destructor of AtaRenameColumn for cleaner C++ code, and wrapping constraint_list in a std::unique_ptr to guarantee exception safety during assignment.

Comment on lines +117 to +122
ColumnConstraintList::iterator itr;

for (itr = fConstraints.begin(); itr != fConstraints.end(); ++itr)
{
delete *itr;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using a range-based for loop instead of an explicit iterator makes the code more concise, readable, and idiomatic in modern C++.

  for (auto* constraint : fConstraints)
  {
    delete constraint;
  }

Comment thread dbcon/ddlpackage/ddlpkg.h
Comment on lines +873 to +877
{
fConstraints = *constraint_list;
delete constraint_list; // fConstraints now owns the element pointers; free the redundant list
// container the parser allocated (mirrors ColumnDef's constructor).
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If fConstraints = *constraint_list; throws an exception (such as std::bad_alloc during container copy), the delete constraint_list; statement will be bypassed, resulting in a memory leak of the constraint_list container. Wrapping the raw pointer in a std::unique_ptr immediately ensures exception-safe cleanup.

    {
      std::unique_ptr<ColumnConstraintList> auto_delete(constraint_list);
      fConstraints = *auto_delete;
    }

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