Skip to content

MDEV-39513 connect table_type=INI memory leak#5056

Open
grooverdan wants to merge 1 commit intoMariaDB:11.4from
grooverdan:MDEV-39513
Open

MDEV-39513 connect table_type=INI memory leak#5056
grooverdan wants to merge 1 commit intoMariaDB:11.4from
grooverdan:MDEV-39513

Conversation

@grooverdan
Copy link
Copy Markdown
Member

The MRUProfile structure, of which CurProfile=MRUProfile[0] can contain a filename that is allocated. It is possible for the CurProfile to be null, while others are allocated, including those with a MRUProfile.filename already malloc.

As such the full PROFILE_ReleaseFile needs to be called on all non-null MRUProfile entries to prevent a memory leak.

Corrects MDEV-9997

The MRUProfile structure, of which CurProfile=MRUProfile[0]
can contain a filename that is allocated. It is possible
for the CurProfile to be null, while others are allocated,
including those with a MRUProfile.filename already malloc.

As such the full PROFILE_ReleaseFile needs to be called on
all non-null MRUProfile entries to prevent a memory leak.

Corrects MDEV-9997
@grooverdan grooverdan requested a review from sanja-byelkin May 8, 2026 07:32
@grooverdan grooverdan added the MariaDB Foundation Pull requests created by MariaDB Foundation label May 8, 2026
Copy link
Copy Markdown

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

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 addresses a memory leak in the CONNECT engine's INI table type (MDEV-39513) by ensuring all cached profiles are released and freed during PROFILE_End. Feedback highlights the need to nullify pointers in the MRUProfile array to prevent potential use-after-free issues and points out a portability issue in the new test case due to a hardcoded absolute path in the result file.

Comment on lines 625 to 634
for (i = 0; i < N_CACHED_PROFILES; i++) {
CurProfile = MRUProfile[i];
if (!CurProfile)
continue;
if (trace(3))
htrc("MRU=%s i=%d\n", SVP(MRUProfile[i]->filename), i);
htrc("MRU=%s i=%d\n", SVP(CurProfile->filename), i);

// CurProfile = MRUProfile[i]; Sergey Vojtovich
// PROFILE_ReleaseFile(); see MDEV-9997
free(MRUProfile[i]);
PROFILE_ReleaseFile();
memfree(CurProfile);
} // endfor i
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current implementation leaves dangling pointers in the MRUProfile array. While memfree(CurProfile) nullifies the global CurProfile pointer (assuming memfree is the standard CONNECT macro), it does not affect the MRUProfile[i] entry. This could lead to use-after-free or double-free issues if PROFILE_End is called multiple times or if the cache is accessed later. It is safer to nullify the array element directly and ensure the global CurProfile is also cleared. Additionally, the closing brace indentation should be corrected to align with the for statement.

  for (i = 0; i < N_CACHED_PROFILES; i++) {
    CurProfile = MRUProfile[i];
    if (!CurProfile)
      continue;

    if (trace(3))
      htrc("MRU=%s i=%d\n", SVP(CurProfile->filename), i);

    PROFILE_ReleaseFile();
    memfree(MRUProfile[i]);
    CurProfile = NULL;
  }


--source include/force_restart.inc

--eval CREATE TABLE t (c INT) ENGINE=CONNECT table_type=INI file_name='$MYSQL_TMP_DIR/foobar';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The test result file contains a hardcoded absolute path (/home/dan/...), which makes the test non-portable and will cause failures on other systems. Use --replace_result to mask the dynamic path. Also, consider removing the trailing semicolon inside the --eval string to avoid the double semicolon (;;) appearing in the result file.

--replace_result $MYSQL_TMP_DIR MYSQL_TMP_DIR
--eval CREATE TABLE t (c INT) ENGINE=CONNECT table_type=INI file_name='$MYSQL_TMP_DIR/foobar'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MariaDB Foundation Pull requests created by MariaDB Foundation

Development

Successfully merging this pull request may close these issues.

2 participants