Skip to content

Commit

Permalink
Added checking to protect against simultaneous double free in safemalloc
Browse files Browse the repository at this point in the history
If two threads would call sf_free() / free_memory() at the same time,
bad_ptr() would not detect this. Fixed by adding extra detection
when working with the memory region under sf_mutex.

Other things:
- If safe_malloc crashes while mutex is hold, stack trace printing will
  hang because we malloc is called by my_open(), which is used by stack
  trace printing code. Fixed by adding MY_NO_REGISTER flag to my_open,
  which will disable the malloc() call to remmeber the file name.
  • Loading branch information
montywi authored and vuvova committed May 19, 2021
1 parent 744a538 commit 79d9a72
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 6 deletions.
1 change: 1 addition & 0 deletions include/my_sys.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ C_MODE_START
#define MY_SHORT_WAIT 64U /* my_lock() don't wait if can't lock */
#define MY_FORCE_LOCK 128U /* use my_lock() even if disable_locking */
#define MY_NO_WAIT 256U /* my_lock() don't wait at all */
#define MY_NO_REGISTER 8196U /* my_open(), no malloc for file name */
/*
If old_mode is UTF8_IS_UTF8MB3, then pass this flag. It mean utf8 is
alias for utf8mb3. Otherwise utf8 is alias for utf8mb4.
Expand Down
2 changes: 1 addition & 1 deletion mysys/my_open.c
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ File my_register_filename(File fd, const char *FileName, enum file_type
if ((int) fd >= MY_FILE_MIN)
{
my_atomic_add32_explicit(&my_file_opened, 1, MY_MEMORY_ORDER_RELAXED);
if ((uint) fd >= my_file_limit)
if ((uint) fd >= my_file_limit || (MyFlags & MY_NO_REGISTER))
DBUG_RETURN(fd);
my_file_info[fd].name = my_strdup(key_memory_my_file_info, FileName, MyFlags);
statistic_increment(my_file_total_opened,&THR_LOCK_open);
Expand Down
17 changes: 15 additions & 2 deletions mysys/safemalloc.c
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ static void *sf_min_adress= (void*) (intptr)~0ULL,
static struct st_irem *sf_malloc_root = 0;

#define MAGICSTART 0x14235296 /* A magic value for underrun key */
#define MAGICEND 0x12345678 /* Value for freed block */

#define MAGICEND0 0x68 /* Magic values for overrun keys */
#define MAGICEND1 0x34 /* " */
Expand Down Expand Up @@ -255,6 +256,7 @@ static void print_stack(void **frame)
static void free_memory(void *ptr)
{
struct st_irem *irem= (struct st_irem *)ptr - 1;
size_t end_offset;

if ((irem->flags & MY_THREAD_SPECIFIC) && irem->thread_id &&
irem->thread_id != sf_malloc_dbug_id())
Expand All @@ -266,6 +268,14 @@ static void free_memory(void *ptr)
}

pthread_mutex_lock(&sf_mutex);
/* Protect against double free at same time */
if (irem->marker != MAGICSTART)
{
pthread_mutex_unlock(&sf_mutex); /* Allow stack trace alloc mem */
DBUG_ASSERT(irem->marker == MAGICSTART); /* Crash */
pthread_mutex_lock(&sf_mutex); /* Impossible, but safer */
}

/* Remove this structure from the linked list */
if (irem->prev)
irem->prev->next= irem->next;
Expand All @@ -277,10 +287,13 @@ static void free_memory(void *ptr)

/* Handle the statistics */
sf_malloc_count--;

irem->marker= MAGICEND; /* Double free detection */
pthread_mutex_unlock(&sf_mutex);

/* only trash the data and magic values, but keep the stack trace */
TRASH_FREE((uchar*)(irem + 1) - 4, irem->datasize + 8);
/* Trash the data and magic values, but keep the stack trace */
end_offset= sizeof(*irem) - ((char*) &irem->marker - (char*) irem);
TRASH_FREE((uchar*)(irem + 1) - end_offset, irem->datasize + 4 + end_offset);
free(irem);
return;
}
Expand Down
7 changes: 4 additions & 3 deletions sql/signal_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ static inline void output_core_info()
(int) len, buff);
}
#ifdef __FreeBSD__
if ((fd= my_open("/proc/curproc/rlimit", O_RDONLY, MYF(0))) >= 0)
if ((fd= my_open("/proc/curproc/rlimit", O_RDONLY, MYF(MY_NO_REGISTER))) >= 0)
#else
if ((fd= my_open("/proc/self/limits", O_RDONLY, MYF(0))) >= 0)
if ((fd= my_open("/proc/self/limits", O_RDONLY, MYF(MY_NO_REGISTER))) >= 0)
#endif
{
my_safe_printf_stderr("Resource Limits:\n");
Expand All @@ -78,7 +78,8 @@ static inline void output_core_info()
my_close(fd, MYF(0));
}
#ifdef __linux__
if ((fd= my_open("/proc/sys/kernel/core_pattern", O_RDONLY, MYF(0))) >= 0)
if ((fd= my_open("/proc/sys/kernel/core_pattern", O_RDONLY,
MYF(MY_NO_REGISTER))) >= 0)
{
len= my_read(fd, (uchar*)buff, sizeof(buff), MYF(0));
my_safe_printf_stderr("Core pattern: %.*s\n", (int) len, buff);
Expand Down

0 comments on commit 79d9a72

Please sign in to comment.