MDEV-39513 connect table_type=INI memory leak#5056
MDEV-39513 connect table_type=INI memory leak#5056grooverdan wants to merge 1 commit intoMariaDB:11.4from
Conversation
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
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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'
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