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

[22 May 2009 10:08] Shane Bester
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
[22 May 2009 11:36] MySQL Verification Team
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.

[26 May 2009 18:26] MySQL Verification Team
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.
[9 Jun 2009 9:32] Christoffer Hall
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?
[12 Jun 2009 14:46] MySQL Verification Team
Shane,

Your testcase does not happen to have CREATE TABLE commands, but only INSERTs ...
[12 Jun 2009 14:54] MySQL Verification Team
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.
[13 Jun 2009 4:56] MySQL Verification Team
sinisa, the reason it doesnt have a create table is because it is not relevent.
[22 Jun 2009 11:45] Christoffer Hall
Hi Shane

Can you still reproduce this bug?
[22 Jun 2009 11:52] MySQL Verification Team
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)
[22 Jun 2009 11:54] MySQL Verification Team
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
[22 Jun 2009 11:55] MySQL Verification Team
no, i never tried sinisa's patch.  this is unmodified sources i'm testing.
[25 Jun 2009 9:02] Christoffer Hall
I've retried on 5.0, 5.1 and 6.0. Cannot repeat across the board on my system.
[27 Jun 2009 1:03] Davi Arnaut
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.
[30 Jun 2009 15:04] MySQL Verification Team
After applying Sinisa's proposed patch the valgrind warning is prevented.
[28 Jul 2009 18:36] Bugs System
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.
[4 Aug 2009 13:55] Bugs System
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)
[4 Aug 2009 19:52] Bugs System
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)
[4 Aug 2009 20:45] Bugs System
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)
[8 Aug 2009 0:42] Paul DuBois
Noted in 5.0.85, 5.1.38, 5.4.4 changelogs.

Invalid memory reads could occur using the compressed client/server
protocol.
[12 Aug 2009 22:24] Paul DuBois
Noted in 5.4.2 changelog because next 5.4 version will be 5.4.2 and not 5.4.4.
[15 Aug 2009 1:43] Paul DuBois
Ignore previous comment about 5.4.2.
[1 Oct 2009 5:59] Bugs System
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)
[1 Oct 2009 7:25] Bugs System
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)
[1 Oct 2009 13:25] Bugs System
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)
[5 Oct 2009 10:50] Bugs System
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)
[7 Oct 2009 19:12] Paul DuBois
The 5.4 fix has been pushed to 5.4.2.