MySQL Bugs: #114338: Client might receive incorrect data in ps-protocol

Bug #114338	Client might receive incorrect data in ps-protocol
Submitted:	13 Mar 2024 16:41	Modified:	14 Mar 2024 11:00
Reporter:	Wang Zhengmao (OCA)	Email Updates:
Status:	Verified	Impact on me:	None
Category:	MySQL Server: Prepared statements	Severity:	S3 (Non-critical)
Version:	8.0	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
When sending time data type in ps-protocol, if server cannot allocate new buffer on the heap, data got by client would be wrong. Even crashes may occur in debug mode

How to repeat:
In fact, you need a hack way to repeat the bug.

step 1: 
Add code in Protocol_binary::store_time function and compile in debug mode.
--- a/sql/protocol_classic.cc
+++ b/sql/protocol_classic.cc
@@ -3950,6 +3950,7 @@ bool Protocol_binary::store_time(const MYSQL_TIME &tm, uint precision) {
     length = 0;
 
   char *pos = packet->prep_append(length + 1, PACKET_BUFFER_EXTRA_ALLOC);
+  DBUG_EXECUTE_IF( "simulate_malloc_fail", { pos = nullptr; } );
   if (pos == nullptr) return false;
   *pos++ = char(length);

step 2: create a table 
create table t1 (c_time time(3));

step 3: insert a record
insert into t1 value (15:24:25.123);

step 4: set debug
set debug = "+d,simulate_malloc_fail";

step 5: run select query in ps-protocol
select * from t1;

Suggested fix:
--- a/sql/protocol_classic.cc
+++ b/sql/protocol_classic.cc
@@ -3950,7 +3950,7 @@ bool Protocol_binary::store_time(const MYSQL_TIME &tm, uint precision) {
     length = 0;
 
   char *pos = packet->prep_append(length + 1, PACKET_BUFFER_EXTRA_ALLOC);
-  if (pos == nullptr) return false;
+  if (pos == nullptr) return true;
   *pos++ = char(length);
 
   const char *const end = pos + length;

Hi MR. Zhengmao,

Thank you for your bug report.

However, first of all, when malloc fails, that is a severe problem with OS, it's malloc library or with particular configuration of MySQL.

We understand that this is a bug report about what happens when there is no longer available memory on the system ..... In that case, most operating systems would just kill the process. In that case, what pos pointer returns is totally irrelevant.

We gladly accept reports based on the code analysis, but we see no code analysis here.

Also, since this is done only with prepared statements , we cannot repeat what you are reporting. We tried with some prepared statements in SQL, with lot's of memory being used, but we still could not reproduce it.

Can't repeat.

For code in Protocol_binary::store_time function
  pos = packet->prep_append(length + 1, PACKET_BUFFER_EXTRA_ALLOC);
  if (pos == nullptr) return false;

First, for return value of Protocol_binary::store_time function, False represents success, true represents failure.

Second, If retrun value of String::prep_append(size_t arg_length, size_t step_alloc)) function is nullptr, meaning an error occurred when attempting to allocate memory or memory allocation length exceeded allowed limit (4GB) for String Class.

Last, if pos is assigned as nullptr, indicating that an error occurred during String::prep_append(), Protocol_binary::store_time should return true instead of false.

Hi Mr. Zhengmao,

Thank you for your feedback.

We have analysed the code that you described and we agree with you.

This is indeed a small bug in the prepared statements protocol.

This is now a verified bug report for the server-side of the prepared statements.

It affects version 8.0 and higher.

Thank you for your contribution.

Contributed patch (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: bug114338.diff (application/octet-stream, text), 1.47 KiB.

Hi Mr. Zhengman,

Thank you for your contribution.

We shall deliver your patch to our Development team.