MySQL Bugs: #87203: Suspicious UAF at storage/innobase/row/row0merge.cc

Bug #87203	Suspicious UAF at storage/innobase/row/row0merge.cc
Submitted:	26 Jul 2017 10:03	Modified:	27 Jul 2017 14:43
Reporter:	alex chen	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S3 (Non-critical)
Version:	5.7	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
Hi all, 

Our code scanner has pointed out a suspicious UAF at storage/innobase/row/row0merge.cc 

At https://github.com/mysql/mysql-server/blob/5.7/storage/innobase/row/row0merge.cc#L1946 
// =============================================== 
	/* Move to the successor of the
	original record. */
	if (!btr_pcur_move_to_next_user_rec(
		    &pcur, &mtr)) {
end_of_index:
		row = NULL;
		mtr_commit(&mtr);
		mem_heap_free(row_heap);
		ut_free(nonnull);
		goto write_buffers;  // Step 1: clean nonnull & row_heap, then start write_buffers 
        } 
// ============================================ 

https://github.com/mysql/mysql-server/blob/5.7/storage/innobase/row/row0merge.cc#L2445 
// ============================================ 
	if (row_merge_file_create_if_needed(
		file, tmpfd,
		buf->n_tuples, path) < 0) {
		err = DB_OUT_OF_MEMORY;
		trx->error_key_num = i;
		goto func_exit;            // Step 2: GOTO func_exit here 
	}

// ============================================== 

https://github.com/mysql/mysql-server/blob/5.7/storage/innobase/row/row0merge.cc#L2515 
// =============================================== 
func_exit:
	/* row_merge_spatial_rows may have committed
	the mtr	before an error occurs. */
	if (mtr.is_active()) {
		mtr_commit(&mtr);
	}
	mem_heap_free(row_heap);
	ut_free(nonnull);              // Step 3: free nonnull & row_heap again. 
// ================================================== 

there are bunch of code that I cannot understand them all, Is that Step 1 & Step 2 exclude each other? or Is there are any chance that a `goto func_exit` will run after Step 1 had happened? 

If there is one path start from step 1, and `goto func_exit`, then I believe it would be a UAF bug, otherwise it would be a false alarm. 

Regards, 

SourceBrella Inc. Alex

How to repeat:
-

HI!

In order to proceed, I need two informations from you.

First of all, what is this MySQL version exactly that you are using ?????

Second, you use acronym UAF. What does it exactly stands for you ????

Thanks @Sinisa Milivojevic

We focused on the source code, I believe the situation is in the master branch of version 5.7, https://github.com/mysql/mysql-server/tree/5.7 
though the pieces of code are shared in serval versions.

Sorry for the term UAF, it stands for Use After Free (https://cwe.mitre.org/data/definitions/416.html), it means, at this situation, Could there are any chance that Step 3 frees the memory already freed by Step 1 (is a possible execution path ) ?

Hi!

Yes, you are right that the function can be called twice. However, as it is the case with most of our memory management functions / methods, each block in heap is set to NULL on the release of the memory and checked for NULL before the release occurs.

That means that you can call this function three times in a row on the same heap object without any danger.

Hi,

thanks, it would be no problem if there is NULL assignment in memory release, but I couldn't sure about that,

there are two suspicious lines, 		

mem_heap_free(row_heap);      // line 1
ut_free(nonnull);                        // line 2

for line 1,
I can see there is a MAGIC number checking when freeing the blocks of a heap, but it seems cannot alter the address stored in variables `row_heap`, the function declaration is [void mem_heap_free(mem_heap_t* heap)] ? `row_heap` still keeps the address ?

for line 2,
ut_free is a macro of the following `deallocate` function (according to my code cross references), I couldn't find a NULL assignment inside, could it still be a problem ?
https://github.com/mysql/mysql-server/blob/5.7/storage/innobase/include/ut0new.h#L391

	/** Free a memory allocated by allocate() and trace the deallocation.
	@param[in,out]	ptr		pointer to memory to free
	@param[in]	n_elements	number of elements allocated (unused) */
	void
	deallocate(
		pointer		ptr,
		size_type	n_elements = 0)
	{
		if (ptr == NULL) {
			return;
		}

#ifdef UNIV_PFS_MEMORY
		ut_new_pfx_t*	pfx = reinterpret_cast<ut_new_pfx_t*>(ptr) - 1;

		deallocate_trace(pfx);

		free(pfx);
#else
		free(ptr);
#endif /* UNIV_PFS_MEMORY */
	}

We did lots of analysis and concluded that ut_free() is idempotent. We are sure that it is so in non-debug mode. Debug mode still remains to be analysed, as the code path is quite different.

So, this remains as "Not a Bug".

Hi!

The analysis of the code in debug mode proves that there is an improbable, but possible case where non-NULL pointer could be freed twice.

Hence, this bug is also verified.

Thank you for your contribution.

Hi,

Thank you for your great efforts.