Skip to content

Commit 4e6769f

Browse files
author
Eric Wong
committed
avoid redundant close from calling mysql_close
Calling close redundantly on an FD means a race condition will break multithreaded applications (or worse, lead to inadvertant information disclosure). Consider the following multithreaded timeline with two threads: thread 1 (mysql2 close) | thread 2 (blocking accept loop) ---------------------------------+---------------------------------- close(9) -> fd=9 released | | accept() -> fd=9 acquired mysql_close -> close(9) | # releases fd=9 which thread 2 | # just took using accept() | | read(9) -> EBADF! We must prevent mysql_close from possibly hitting a recycled FD when it calls close again. So we redirect the existing FD to a dummy socket (atomically closing the original FD/socket in the process). This means mysql_close will call close on a valid FD, but that FD now points to a dummy socket and not a valid FD. I considered using socketpair (to ensure write/shutdown inside mysql_close succeeds), but decided against it as there may be a chance the client library will one day wait for a disconnect message from the server. Thanks to Evan Miller <[email protected]> for this idea.
1 parent fe628cc commit 4e6769f

File tree

1 file changed

+42
-7
lines changed

1 file changed

+42
-7
lines changed

ext/mysql2/client.c

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
#include <errno.h>
44
#ifndef _WIN32
5+
#include <sys/types.h>
56
#include <sys/socket.h>
67
#endif
78
#include <unistd.h>
@@ -162,24 +163,58 @@ static void *nogvl_connect(void *ptr) {
162163
return (void *)(client ? Qtrue : Qfalse);
163164
}
164165

166+
#ifndef _WIN32
167+
/*
168+
* Redirect clientfd to a dummy socket for mysql_close to
169+
* write, shutdown, and close on as a no-op.
170+
* We do this hack because we want to call mysql_close to release
171+
* memory, but do not want mysql_close to drop connections in the
172+
* parent if the socket got shared in fork.
173+
* Returns Qtrue or false (success or failure)
174+
*/
175+
static VALUE invalidate_fd(int clientfd)
176+
{
177+
/* TODO: set cloexec flags, atomically if possible */
178+
int sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
179+
180+
if (sockfd < 0) {
181+
/*
182+
* Cannot raise here, because one or both of the following may be true:
183+
* a) we have no GVL (in C Ruby)
184+
* b) are running as a GC finalizer
185+
*/
186+
return Qfalse;
187+
}
188+
189+
dup2(sockfd, clientfd);
190+
close(sockfd);
191+
192+
return Qtrue;
193+
}
194+
#endif /* _WIN32 */
195+
165196
static void *nogvl_close(void *ptr) {
166197
mysql_client_wrapper *wrapper;
167198
wrapper = ptr;
168199
if (wrapper->connected) {
169200
wrapper->active_thread = Qnil;
170201
wrapper->connected = 0;
171202
#ifndef _WIN32
172-
/* Call close() on the socket before calling mysql_close(). This prevents
203+
/* Invalidate the socket before calling mysql_close(). This prevents
173204
* mysql_close() from sending a mysql-QUIT or from calling shutdown() on
174-
* the socket. The difference is that close() will drop this process's
175-
* reference to the socket only, while a QUIT or shutdown() would render
176-
* the underlying connection unusable, interrupting other processes which
177-
* share this object across a fork().
205+
* the socket. The difference is that invalidate_fd will drop this
206+
* process's reference to the socket only, while a QUIT or shutdown()
207+
* would render the underlying connection unusable, interrupting other
208+
* processes which share this object across a fork().
178209
*/
179-
close(wrapper->client->net.fd);
210+
if (invalidate_fd(wrapper->client->net.fd) == Qfalse) {
211+
fprintf(stderr, "[WARN] mysql2 failed to invalidate FD safely, leaking some memory\n");
212+
close(wrapper->client->net.fd);
213+
return NULL;
214+
}
180215
#endif
181216

182-
mysql_close(wrapper->client);
217+
mysql_close(wrapper->client); /* only used to free memory at this point */
183218
}
184219

185220
return NULL;

0 commit comments

Comments
 (0)