MySQL Bugs: #110302: The parameter 'trx' in function 'trx

Bug #110302	The parameter 'trx' in function 'trx_free' may cause dangling pointer
Submitted:	8 Mar 12:15	Modified:	14 Mar 2:04
Reporter:	jie xu	Email Updates:
Status:	Not a Bug	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S3 (Non-critical)
Version:	8.0	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	transaction

Description:
  The parameter 'trx' in function 'trx_free' is set to NULL, and this parameter pass by reference。

  But the parameter 'trx' in function 'trx_free_resurrected/trx_free_for_background/trx_free_prepared_or_active_recovered' pass by value; and 'trx_free' is called by above functions.

  When some where use above those functions to free 'trx', the 'trx' become to dangling pointer; and this behaviour is very dangerous.

How to repeat:
Read latest MySQL8.0 code.

Suggested fix:
1. the parameter 'trx' in function 'trx_free_resurrected/trx_free_for_background/trx_free_prepared_or_active_recovered' pass by reference, and when use above functions to free 'trx', must use reference, not local variable.

2. remove ’trx = nullptr;‘ in function 'trx_free', and this parameter pass by value; when after use functions 'trx_free_resurrected/trx_free_for_background/trx_free_prepared_or_active_recovered', set variable 'trx' to 'nullptr'.

Hi Mr. xu,

Thank you for your bug report.

It sounds interesting.

However, information provided by you is insufficient ......

Please, provide the relevant excerpts from all the mentioned functions in the latest 8.0 releases.

After that, please provide excerpts rom the same functions, from some older 8.0 releases, where it was used correctly.

Thank you very much, in advance.

======================================================================================
/**
Release a trx_t instance back to the pool.
@param trx the instance to release. */
static void trx_free(trx_t *&trx) { // pass parameter by reference.
  ...

  trx = nullptr; // set parameter ’trx‘ to nullptr.
}

=========================================================================================
The function 'trx_free' pass parameter by reference, and set it to nullptr.

=========================================================================================
/** Free and initialize a transaction object instantiated during recovery.
@param[in,out]  trx     transaction object to free and initialize */
void trx_free_resurrected(trx_t *trx) { // pass parameter by value.
  trx_validate_state_before_free(trx);

  trx_init(trx);

  trx_free(trx); // call trx_free.
}

------------------------------------------------------------------------
/** Free a transaction that was allocated by background or user threads.
@param[in,out]  trx     transaction object to free */
void trx_free_for_background(trx_t *trx) { // pass parameter by value.
  trx_validate_state_before_free(trx);

  trx_free(trx); // call trx_free.
}

------------------------------------------------------------------------
void trx_free_prepared_or_active_recovered(trx_t *trx) { // pass parameter by value.
  ...

  trx_free(trx); // call trx_free.
}

=========================================================================================
Three above functions call 'trx_free', and pass parameter by value.

When use three above functions to free transaction, the transaction become dangling pointer.

Hi Mr. xu,

We have examined your code and found out that this is not a bug.

Some of the functions that you presented are totally rewritten in latest 8.0 releases.

In your second example, a pointer to the structure is passed in the parameter. A pointer to the structure is actually passing by reference. Since that structure was allocated from the heap, passing the pointer to the struct in  free() is perfectly OK.

And so on and so forth .....

Not a bug.

Thanks for your reply!!

I know what you mean.

I also read latest MySQL 8.0 code.

Let's checkout this example.
======================================================================================

void test_free_trx() {
    trx_t *trx = trx_allocate_for_background();

    // Do something.

    trx_free_for_background(trx);
}

======================================================================================
After call trx_free_for_background(trx); trx‘s resource freed by function 'trx_free()', and variable 'trx' would become dangling pointer since function 'trx_free_for_background()' pass parameter by value.
Fortunately, I checkout latest MySQL 8.0 code, 'trx' never be used after called 'trx_free_for_background(trx)'.

If you think this is normal, the function 'trx_free()' pass parameter by reference is unnecessary, and set 'trx' to 'nullptr' in 'trx_free()' is useless.

Hi,

Once again.

That variable is a pointer to the structure, and that pointer is allocated on the heap.

That pointer is then for the deallocation, which is fine. You do not need to pass to free() a pointer to the pointer to the structure.

Since that pointer is a local variable that is freed in the last line of the function, then there is nothing wrong.

Thanks for you reply.

I will accept this explanation

But I still think the implement of function 'trx_free()' isn't very well.

We agree that it could be improved.

Actually , any code ever written could be improved.

However, code restructuring and improvement is not considered a bug.

Thanks for you reply again.

I agree with you.