MySQL Bugs: #45031: invalid memory reads in my_real

Bug #45031	invalid memory reads in my_real_read using protocol compression
Submitted:	22 May 2009 10:08	Modified:	8 Aug 2009 0:42
Reporter:	Shane Bester (Platinum Quality Contributor)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: General	Severity:	S2 (Serious)
Version:	5.0.82, 5.1.34, 5.1.35, 5.1.37, 6.0.12	OS:	Linux (32-bit fc8)
Assigned to:	Alexey Kopytov	CPU Architecture:	Any
Tags:	compressed protocol, valgrind

Description:
was running my application testsuite against 6.0.12-debug under valgrind:

gypsy --queryfile=qa.sql --compressed-protocol=1 --shuffle-queries=1 --duration=300 

and found this error:

 Thread 14:
 Invalid read of size 4
at : my_real_read(st_net*, unsigned*) (net_serv.cc:950)
by : my_net_read (net_serv.cc:1122)
by : do_command(THD*) (sql_parse.cc:692)
by : handle_one_connection (sql_connect.cc:1146)
by : start_thread (in /lib/libpthread-2.5.so)
by : clone (in /lib/libc-2.5.so)
  Address 0xE9CC28C is 540,676 bytes inside a block of size 540,679 alloc'd
at : realloc (vg_replace_malloc.c:306)
by : my_realloc (my_realloc.c:62)
by : net_realloc (net_serv.cc:193)
by : my_real_read(st_net*, unsigned*) (net_serv.cc:961)
by : my_net_read (net_serv.cc:1122)
by : do_command(THD*) (sql_parse.cc:692)
by : handle_one_connection (sql_connect.cc:1146)
by : start_thread (in /lib/libpthread-2.5.so)
by : clone (in /lib/libc-2.5.so)

The line 950 in net_serve.cc is this:

#ifdef HAVE_COMPRESS
if (net->compress)
{
  /*
    If the packet is compressed then complen > 0 and contains the
    number of bytes in the uncompressed packet
  */
  *complen=uint3korr(&(net->buff[net->where_b + NET_HEADER_SIZE])); <---HERE
}
#endif

How to repeat:
will make testcase later if I see this again

mysqld in valgrind, then extract and import: mysql -uroot --compress -h127.0.0.1 test <bug45031.sql

Attachment: bug45031.sql.bz2 (application/x-bzip2, text), 335.42 KiB.

I quickly checked in a debugger on windoze.  When importing testcase:
Made a quick patch to print out some infos:

if (net->compress)
{
  /*
    If the packet is compressed then complen > 0 and contains the
    number of bytes in the uncompressed packet
  */
  printf("net->where_b + NET_HEADER_SIZE = 0x%x \n",net->where_b + NET_HEADER_SIZE);
  printf("net->buff = %p \n",net->buff);
  printf("net->buf_length = %lu \n",net->buf_length);
  printf("net->buff_end = %p \n",net->buff_end);

  *complen=uint3korr(&(net->buff[net->where_b + NET_HEADER_SIZE]));
  printf("complen = %lu\n",*complen);
  printf("\n\n");
}

net->where_b + NET_HEADER_SIZE = 0x4004
net->buff = 0000000004C6E478
net->buf_length = 37
net->buff_end = 0000000004C72478
complen = 536789

So, we see that 4C6E478 + 4004 = 4C7247C
This is 4 bytes over net->buff_end.

Keep in mind also that uint3korr reads 3 bytes on some platforms and on
others, 4 bytes is read.

Hi Shane

I tried to repeat this bug in mysql-5.0-bugteam, mysql-5.1-bugteam and mysql-6.0-bugteam. I cannot repeat the bug in either of them. Do you still get the bug?

Shane,

Your testcase does not happen to have CREATE TABLE commands, but only INSERTs ...

Shane,

If you can repeat the bug, please, try this patch:

=== modified file 'sql/net_serv.cc'
--- sql/net_serv.cc	2009-02-10 22:47:54 +0000
+++ sql/net_serv.cc	2009-06-12 14:57:24 +0000
@@ -930,7 +930,11 @@
 	len=uint3korr(net->buff+net->where_b);
 	if (!len)				/* End of big multi-packet */
 	  goto end;
-	helping = max(len,*complen) + net->where_b;
+	helping = max(len,*complen) + net->where_b + NET_HEADER_SIZE;
+#ifdef HAVE_COMPRESS
+	if (net->compress)
+	  helping += COMP_HEADER_SIZE;
+#endif
 	/* The necessary size of net->buff */
 	if (helping >= net->max_packet)
 	{

I studied the behaviour and it seems that this bug is caused by very small differences in the sizes of compressed versus uncompressed data streams. The above patch is for 5.0.

sinisa, the reason it doesnt have a create table is because it is not relevent.

Hi Shane

Can you still reproduce this bug?

Yup, on latest bzr version 5.1.37, revision 2951 :

==29986== Invalid read of size 4
==29986==    at 0x81ED88A: my_real_read(st_net*, unsigned*) (net_serv.cc:928)
==29986==    by 0x81EDD60: my_net_read (net_serv.cc:1097)
==29986==    by 0x821383F: do_command(THD*) (sql_parse.cc:796)
==29986==    by 0x8202002: handle_one_connection (sql_connect.cc:1127)
==29986==    by 0x4893DA: start_thread (in /lib/libpthread-2.5.so)
==29986==    by 0x3D606D: clone (in /lib/libc-2.5.so)
==29986==  Address 0x65E4DFC is 16,388 bytes inside a block of size 16,391 alloc'd
==29986==    at 0x40054FB: realloc (vg_replace_malloc.c:306)
==29986==    by 0x84F682A: my_realloc (my_realloc.c:62)
==29986==    by 0x81ED559: net_realloc (net_serv.cc:191)
==29986==    by 0x81ED8D4: my_real_read(st_net*, unsigned*) (net_serv.cc:939)
==29986==    by 0x81EDD60: my_net_read (net_serv.cc:1097)
==29986==    by 0x821383F: do_command(THD*) (sql_parse.cc:796)
==29986==    by 0x8202002: handle_one_connection (sql_connect.cc:1127)
==29986==    by 0x4893DA: start_thread (in /lib/libpthread-2.5.so)
==29986==    by 0x3D606D: clone (in /lib/libc-2.5.so)

valgrind-3.2.1

 valgrind --tool=memcheck --leak-check=full   -v --show-reachable=yes   ./bin/mysqld  --no-defaults   --skip-grant-tables --skip-name-resolve  --socket=sock.sock --federated  --basedir=. --datadir=./data 

mysql --compress --force -uroot -h192.168.250.4 test < bug45031.sql

no, i never tried sinisa's patch.  this is unmodified sources i'm testing.

I've retried on 5.0, 5.1 and 6.0. Cannot repeat across the board on my system.

Beware that the uint3korr version that reads 4 bytes is only active for non-valgrind 32 bits builds:

#define uint3korr(A)    (long) (*((unsigned int *) (A)) & 0xFFFFFF)

Also, gcc 4.3.3 will transform this into a 3-byte read... valgrind might not be of much use in this case.

=== modified file 'sql/net_serv.cc'
--- sql/net_serv.cc	2009-02-13 16:41:47 +0000
+++ sql/net_serv.cc	2009-06-27 00:41:11 +0000
@@ -925,6 +925,9 @@ my_real_read(NET *net, size_t *complen)
 	    If the packet is compressed then complen > 0 and contains the
 	    number of bytes in the uncompressed packet
 	  */
+          DBUG_ASSERT((net->where_b + NET_HEADER_SIZE + sizeof(unsigned int)) <=
+                      (NET_HEADER_SIZE + COMP_HEADER_SIZE));
+
 	  *complen=uint3korr(&(net->buff[net->where_b + NET_HEADER_SIZE]));
 	}
 #endif

Just connecting with --compress should trigger the assert.

After applying Sinisa's proposed patch the valgrind warning is prevented.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/79445

2787 Alexey Kopytov	2009-07-28
      Bug #45031: invalid memory reads in my_real_read using protocol 
                  compression 
       
      Since uint3korr() may read 4 bytes depending on build flags and 
      platform, allocate 1 extra "safety" byte in the network buffer 
      for cases when uint3korr() in my_real_read() is called to read
      last 3 bytes in the buffer. 
       
      It is practically hard to construct a reliable and reasonably 
      small test case for this bug as that would require constructing 
      input stream such that a certain sequence of bytes in a 
      compressed packet happens to be the last 3 bytes of the network 
      buffer. 
     @ sql/net_serv.cc
        Allocate 1 extra "safety" byte in the network buffer for cases 
        when uint3korr() is used to read last 3 bytes in the buffer.

Pushed into 5.0.85 (revid:davi.arnaut@sun.com-20090804135315-6lfdnk4zjwk7kn7r) (version source revid:davi.arnaut@sun.com-20090804135315-6lfdnk4zjwk7kn7r) (merge vers: 5.0.85) (pib:11)

Pushed into 5.4.4-alpha (revid:alik@sun.com-20090804194615-h40sa098mx4z49qg) (version source revid:alexey.kopytov@sun.com-20090728191448-k6ss9cg1fc89rr9o) (merge vers: 5.4.4-alpha) (pib:11)

Pushed into 5.1.38 (revid:davi.arnaut@sun.com-20090804204317-ggodqkik7de6nfpz) (version source revid:davi.arnaut@sun.com-20090804204317-ggodqkik7de6nfpz) (merge vers: 5.1.38) (pib:11)

Noted in 5.0.85, 5.1.38, 5.4.4 changelogs.

Invalid memory reads could occur using the compressed client/server
protocol.

Noted in 5.4.2 changelog because next 5.4 version will be 5.4.2 and not 5.4.4.

Ignore previous comment about 5.4.2.

Pushed into 5.1.39-ndb-6.3.28 (revid:jonas@mysql.com-20091001055605-ap2kiaarr7p40mmv) (version source revid:jonas@mysql.com-20091001055605-ap2kiaarr7p40mmv) (merge vers: 5.1.39-ndb-6.3.28) (pib:11)

Pushed into 5.1.39-ndb-7.0.9 (revid:jonas@mysql.com-20091001072547-kv17uu06hfjhgjay) (version source revid:jonas@mysql.com-20091001071652-irejtnumzbpsbgk2) (merge vers: 5.1.39-ndb-7.0.9) (pib:11)

Pushed into 5.1.39-ndb-7.1.0 (revid:jonas@mysql.com-20091001123013-g9ob2tsyctpw6zs0) (version source revid:jonas@mysql.com-20091001123013-g9ob2tsyctpw6zs0) (merge vers: 5.1.39-ndb-7.1.0) (pib:11)

Pushed into 5.1.39-ndb-6.2.19 (revid:jonas@mysql.com-20091005103850-dwij2dojwpvf5hi6) (version source revid:jonas@mysql.com-20090930185117-bhud4ek1y0hsj1nv) (merge vers: 5.1.39-ndb-6.2.19) (pib:11)

The 5.4 fix has been pushed to 5.4.2.