MySQL Bugs: #65956: client and libmysqlclient VIO drops connection if signal received during read()

Bug #65956	client and libmysqlclient VIO drops connection if signal received during read()
Submitted:	20 Jul 2012 6:45	Modified:	10 Jan 2013 11:07
Reporter:	David Basden	Email Updates:
Status:	Won't fix	Impact on me:	None
Category:	MySQL Server: C API (client library)	Severity:	S3 (Non-critical)
Version:	5.5.25a, 5.5.27	OS:	Linux (Likely any POSIX.1-2001)
Assigned to:		CPU Architecture:	Any
Tags:	regression

Description:
If the mysql client (or libmysqlclient) is reading from a connected unencrypted socket, and the read() syscall inside vio_read() is interrupted by a signal, the client will close the socket itself, and complain that the connection has been lost.

This will happen if the client process receives any signal, not just those stopping or killing the process. e.g. SIGWINCH (signalling a window size change) will trigger the behaviour in the command-line mysql client.

This seems to be due to vio_read() in viosocket.c not checking to see if errno is set to EINTR if the read() return value is -1. If errno is set to EINTR after a return of -1, the read can be safely retried and does not indicate a socket error.

See http://www.kernel.org/doc/man-pages/online/pages/man7/signal.7.html for more information on the behaviour of read() and others in this case.

How to repeat:
Case 1:

 - On a Linux workstation, open a text terminal window in X11
 - Run the 'mysql' client locally, and connect to a mysql server over an unencrypted TCP socket.
 - Run a long running query such as:
      select count(*) from hosts where name != rand();
 - While the query is running resize the terminal window (sending a SIGWINCH)
 - Query will abort with the error:
   "ERROR 2013 (HY000): Lost connection to MySQL server during query"

Case 2:

Perform the same query using the C library, and then attempt to attach to the client process with 'strace'. For example with python and MySQL-python linked against libmysqlclient18:

  import MySQLdb

  db = MySQLdb.connect(host='localhost',user='user',passwd='pw',db='mydb')
  db.query('select count(*) from hosts where name != rand();')
  r = db.store_result()
  for row in r.fetch_row(maxrows=0): pass

then in another window while that is running, find the client proccess ID and run 'strace -p <process_id>'. The client will then abort the query

Expected behaviour:

 - Client socket connection is unaffected by any benign signals.

Suggested fix:
This is a quick patch to the vio to check for EINTR and retry the read(). It tests fine under load in production.

--- a/vio/viosocket.c   2012-07-19 17:54:19.238520119 +1000
+++ b/vio/viosocket.c   2012-07-19 17:59:40.831071280 +1000
@@ -53,7 +53,12 @@
   r = recv(vio->sd, buf, size,0);
 #else
   errno=0;                                     /* For linux */
-  r = read(vio->sd, buf, size);
+  while (1) {
+    r = read(vio->sd, buf, size);
+    if ((r == (size_t) -1) && (errno == EINTR))
+       continue;
+    break;
+  }
 #endif /* __WIN__ */
 #ifndef DBUG_OFF
   if (r == (size_t) -1)

Maybe a duplicate of http://bugs.mysql.com/bug.php?id=62578

Thank you for the report.

Verified as described. Versions 5.0 and 5.7 are not affected.

The retry logic is at a higher level. If you take a look at my_real_read(), there is a check for the case when read is interrupted due to a signal:

 831│         if ((length= vio_read(net->vio, pos, remain)) <= 0L)
 832│         {
 833├>          my_bool interrupted = vio_should_retry(net->vio);
 834│

If the read() was interrupted with a signal, the read is retried but only up to a certain amount:

 877├>            if (retry_count++ < net->retry_count)
 878│               continue;

This was done on purpose to allow client applications to interrupt threads waiting on socket used by the MySQL client library.

If you do not wish to have this behavior, please increase the retry_count from it's default of 1.

Also, the patch is wrong. It is going to retry any read interrupted by a signal, even when used in the context of the server. In the server, such interruptions are used, for example, to implement killing threads that are waiting to read from a client.

Also, this behavior was changed in MySQL 5.6 and up. For the client library, interrupted I/O operations are now always retried.

From the internal analysis:

5.6 has an implementation where read is done indefinitely in case of socket interrupts(func: net_should_retry in net_serv.cc). Backporting this logic to 5.5 (in my_real_read(NET *net, size_t *complen)) might create some other issues including removing the possibility of developer to react to signals. As this bug is neither a security issue nor a customer bug, it is suggested that it is better not fixed.

Davi , you are right, in 5.6, SA_RESTART makes the system call "recv " locks like blocking.

We occur this problem too, and I find that what makes 5.5 (libmysqlclient.so.18.0.0) different from 5.1(libmysqlclient.so.16.0.0), is that client api in 5.1 , the THREAD_SAFE_CLIENT is defined, so 
#if defined(THREAD_SAFE_CLIENT) && !defined(MYSQL_SERVER)
      if (vio_errno(net->vio) == SOCKET_EINTR)
      {    
        DBUG_PRINT("warning",("Interrupted write. Retrying..."));
        continue;
      }    
#endif /* defined(THREAD_SAFE_CLIENT) && !defined(MYSQL_SERVER) */
makes the client retry, so it is not limited by the net->retry_count.

But in 5.5,  defined(THREAD_SAFE_CLIENT) is *ALWARYS* false, so error comes out.

********
Can anybody tell me how to enable THREAD_SAFE_CLIENT in 5.5? I tried  -DENABLED_THREAD_SAFE_CLIENT=ON, but THREAD_SAFE_CLIENT is not defined yet.
Do I miss anything, or it is a bug in cmake process?

Thanks

Found and tracked in Debian here:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=715065

See also http://bugs.mysql.com/bug.php?id=82019