Permalink
Browse files

Fix segfault when user QUITs while buffered socket data still being w…

…ritten. Fix stuck subuser when previous subuser was still writing data. Also make socket write buffer size a multiple of 2K.
  • Loading branch information...
1 parent 556b7f4 commit a5ab6f8ce997892048705472ed1d5a5d92f48ccd @catwood committed Nov 1, 2012
Showing with 85 additions and 46 deletions.
  1. +20 −6 src/cmd.c
  2. +1 −1 src/proxy.c
  3. +20 −15 src/servers.c
  4. +33 −16 src/sock.c
  5. +2 −2 src/transports.c
  6. +1 −1 src/transports.h
  7. +7 −4 src/users.c
  8. +1 −1 src/users.h
View
@@ -185,16 +185,30 @@ int process_cmd(json_item *ijson, struct _cmd_process *pc, subuser **iuser, acet
struct _transport_open_same_host_p retval = transport_open_same_host(sub, pc->client, pc->guser->transport);
if (retval.client_close != NULL) {
- RAW *newraw;
- json_item *jlist = json_new_object();
+ // Send CLOSE if no response has been sent yet
+ if (!sub->headers.sent) {
+ RAW *newraw;
+ json_item *jlist = json_new_object();
- json_set_property_strZ(jlist, "value", "null");
+ json_set_property_strZ(jlist, "value", "null");
- newraw = forge_raw("CLOSE", jlist);
+ newraw = forge_raw("CLOSE", jlist);
- send_raw_inline((retval.client_close->fd == pc->client->fd ? pc->client : sub->client), pc->transport, newraw, g_ape);
+ send_raw_inline((retval.client_close->fd == pc->client->fd ? pc->client : sub->client), pc->transport, newraw, g_ape);
+ }
- shutdown(retval.client_close->fd, 2);
+ // It's not safe to leave the subuser pointer in co->attach anymore
+ // since subuser could subsequently be deleted, leaving a pointer into free heap.
+ // So, let this socket finish up on its own and pretend its already finished.
+
+ sub->state = ADIED;
+ sub->headers.sent = 0;
+ http_headers_free(sub->headers.content);
+ sub->headers.content = NULL;
+ sub->burn_after_writing = 0;
+
+ g_ape->co[retval.client_close->fd]->attach = NULL;
+ safe_shutdown(retval.client_close->fd, g_ape);
}
sub->client = cp.client = retval.client_listener;
sub->state = retval.substate;
View
@@ -232,7 +232,7 @@ void proxy_shutdown(ape_proxy *proxy, acetables *g_ape)
{
if (proxy->state == PROXY_CONNECTED) {
- shutdown(proxy->sock.fd, 2);
+ safe_shutdown(proxy->sock.fd, g_ape);
proxy->state = PROXY_TOFREE;
}
if (proxy->prev != NULL) {
View
@@ -36,31 +36,36 @@ static void ape_read(ape_socket *co, ape_buffer *buffer, size_t offset, acetable
static void ape_sent(ape_socket *co, acetables *g_ape)
{
- if (co->attach != NULL && ((subuser *)(co->attach))->burn_after_writing) {
- transport_data_completly_sent((subuser *)(co->attach), ((subuser *)(co->attach))->user->transport);
- ((subuser *)(co->attach))->burn_after_writing = 0;
+ subuser *sub = (subuser *)(co->attach);
+ if ((sub != NULL) && (sub->burn_after_writing)) {
+ if (sub->user != NULL) {
+ transport_data_completly_sent(sub, sub->user->transport, g_ape);
+ }
+ sub->burn_after_writing = 0;
}
}
static void ape_disconnect(ape_socket *co, acetables *g_ape)
{
- if (co->attach != NULL) {
+ subuser *sub = (subuser *)(co->attach);
+ if (sub != NULL) {
- if (((subuser *)(co->attach))->wait_for_free == 1) {
- free(co->attach);
+ if (sub->wait_for_free == 1) {
+ free(sub);
co->attach = NULL;
return;
}
- if (co->fd == ((subuser *)(co->attach))->client->fd) {
-
- ((subuser *)(co->attach))->headers.sent = 0;
- ((subuser *)(co->attach))->state = ADIED;
- http_headers_free(((subuser *)(co->attach))->headers.content);
- ((subuser *)(co->attach))->headers.content = NULL;
- if (((subuser *)(co->attach))->user->istmp) {
- deluser(((subuser *)(co->attach))->user, g_ape);
- co->attach = NULL;
+ if (co->fd == sub->client->fd) {
+ sub->headers.sent = 0;
+ sub->state = ADIED;
+ http_headers_free(sub->headers.content);
+ sub->headers.content = NULL;
+ if (sub->user != NULL) {
+ if (sub->user->istmp) {
+ deluser(sub->user, g_ape);
+ co->attach = NULL;
+ }
}
}
View
@@ -40,6 +40,13 @@
#include "log.h"
#include "parser.h"
+// These error codes may have the same value, but POSIX allows them to be different
+#if (EAGAIN == EWOULDBLOCK)
+#define BLOCKING(errnum) (errnum == EWOULDBLOCK)
+#else
+#define BLOCKING(errnum) ((errnum == EWOULDBLOCK) || (errnum == EAGAIN))
+#endif
+
static int sendqueue(int sock, acetables *g_ape);
@@ -222,6 +229,9 @@ void close_socket(int fd, acetables *g_ape)
events_remove(g_ape->events, fd);
close(fd);
+
+ co->fd = 0;
+ co->attach = NULL;
}
/* Create socket struct if not exists */
@@ -389,7 +399,7 @@ unsigned int sockroutine(acetables *g_ape)
if (g_ape->co[active_fd]->burn_after_writing) {
shutdown(active_fd, 2);
- g_ape->co[active_fd]->burn_after_writing = 0;
+ //g_ape->co[active_fd]->burn_after_writing = 0;
}
}
@@ -418,7 +428,7 @@ unsigned int sockroutine(acetables *g_ape)
g_ape->co[active_fd]->buffer_in.data + g_ape->co[active_fd]->buffer_in.length,
g_ape->co[active_fd]->buffer_in.size - g_ape->co[active_fd]->buffer_in.length);
- if (readb == -1 && errno == EAGAIN) {
+ if ((readb == -1) && BLOCKING(errno)) {
if (g_ape->co[active_fd]->stream_type == STREAM_OUT) {
@@ -536,15 +546,20 @@ static int sendqueue(int sock, acetables *g_ape)
while(t_bytes < bufout->buflen) {
n = write(sock, bufout->buf + t_bytes, r_bytes);
- if (n == -1) {
- if (errno == EAGAIN && r_bytes > 0) {
+ if (n < 0) {
+ if (BLOCKING(errno) && (r_bytes > 0)) {
/* Still not complete */
- memmove(bufout->buf, bufout->buf + t_bytes, r_bytes);
- /* TODO : avoid memmove */
- bufout->buflen = r_bytes;
- return 0;
+ if (t_bytes > 0) {
+ memmove(bufout->buf, bufout->buf + t_bytes, r_bytes);
+ /* TODO : avoid memmove */
+ bufout->buflen = r_bytes;
+ }
+ } else {
+ ape_log(APE_ERR, __FILE__, __LINE__, g_ape,
+ "sendqueue() - write(): %s", strerror(errno));
+ printf("Error: Cannot write to socket %i; %s\n", sock, strerror(errno));
}
- break;
+ return 0;
}
t_bytes += n;
r_bytes -= n;
@@ -573,15 +588,15 @@ int sendbin(int sock, const char *bin, unsigned int len, unsigned int burn_after
n = -2;
}
if (n < 0) {
- if ((errno == EAGAIN && r_bytes > 0) || (n == -2)) {
+ if ((n == -2) || (BLOCKING(errno) && (r_bytes > 0))) {
if (g_ape->bufout[sock].buf == NULL) {
- g_ape->bufout[sock].allocsize = r_bytes + 128; /* add padding to prevent extra data to be reallocated */
+ g_ape->bufout[sock].allocsize = (r_bytes + 0x07ff) & (~0x07ff); /* round up modulo 2048 */
g_ape->bufout[sock].buf = xmalloc(sizeof(char) * g_ape->bufout[sock].allocsize);
g_ape->bufout[sock].buflen = r_bytes;
} else {
g_ape->bufout[sock].buflen += r_bytes;
if (g_ape->bufout[sock].buflen > g_ape->bufout[sock].allocsize) {
- g_ape->bufout[sock].allocsize = g_ape->bufout[sock].buflen + 128;
+ g_ape->bufout[sock].allocsize = (g_ape->bufout[sock].buflen + 0x07ff) & (~0x07ff);
g_ape->bufout[sock].buf = xrealloc(g_ape->bufout[sock].buf, sizeof(char) * g_ape->bufout[sock].allocsize);
}
}
@@ -591,11 +606,13 @@ int sendbin(int sock, const char *bin, unsigned int len, unsigned int burn_after
if (burn_after_writing) {
g_ape->co[sock]->burn_after_writing = 1;
}
-
- return 0;
+ } else {
+ ape_log(APE_ERR, __FILE__, __LINE__, g_ape,
+ "sendbin() - write(): %s", strerror(errno));
+ printf("Error: Cannot write to socket %i; %s\n", sock, strerror(errno));
}
- break;
+ return 0;
}
t_bytes += n;
r_bytes -= n;
@@ -614,6 +631,6 @@ void safe_shutdown(int sock, acetables *g_ape)
if (g_ape->bufout[sock].buf == NULL) {
shutdown(sock, 2);
} else {
- g_ape->co[sock]->burn_after_writing = 1;
+ g_ape->co[sock]->burn_after_writing = 2;
}
}
View
@@ -51,13 +51,13 @@ struct _transport_open_same_host_p transport_open_same_host(subuser *sub, ape_so
return ret;
}
-void transport_data_completly_sent(subuser *sub, transport_t transport)
+void transport_data_completly_sent(subuser *sub, transport_t transport, acetables *g_ape)
{
switch(transport) {
case TRANSPORT_LONGPOLLING:
case TRANSPORT_JSONP:
default:
- do_died(sub);
+ do_died(sub, g_ape);
break;
case TRANSPORT_PERSISTANT:
case TRANSPORT_XHRSTREAMING:
View
@@ -46,7 +46,7 @@ typedef enum {
struct _transport_open_same_host_p transport_open_same_host(subuser *sub, ape_socket *client, transport_t transport);
-void transport_data_completly_sent(subuser *sub, transport_t transport);
+void transport_data_completly_sent(subuser *sub, transport_t transport, acetables *g_ape);
void transport_start(acetables *g_ape);
void transport_free(acetables *g_ape);
struct _transport_properties *transport_get_properties(transport_t transport, acetables *g_ape);
View
@@ -223,15 +223,15 @@ void deluser(USERS *user, acetables *g_ape)
}
-void do_died(subuser *sub)
+void do_died(subuser *sub, acetables *g_ape)
{
if (sub->state == ALIVE) {
sub->state = ADIED;
sub->headers.sent = 0;
http_headers_free(sub->headers.content);
sub->headers.content = NULL;
- shutdown(sub->client->fd, 2);
+ safe_shutdown(sub->client->fd, g_ape);
}
}
@@ -259,7 +259,7 @@ void check_timeout(acetables *g_ape, int *last)
/* Data completetly sent => closed */
if (send_raws(*n, g_ape)) {
- transport_data_completly_sent(*n, (*n)->user->transport); // todo : hook
+ transport_data_completly_sent(*n, (*n)->user->transport, g_ape); // todo : hook
} else {
(*n)->burn_after_writing = 1;
@@ -577,10 +577,13 @@ void delsubuser(subuser **current, acetables *g_ape)
clear_properties(&del->properties);
+ del->user = NULL;
+
if (del->state == ALIVE) {
del->wait_for_free = 1;
- do_died(del);
+ do_died(del, g_ape);
} else {
+ g_ape->co[del->client->fd]->attach = NULL;
free(del);
}
View
@@ -216,7 +216,7 @@ USERS *seek_user_simple(const char *nick, acetables *g_ape);
void deluser(USERS *user, acetables *g_ape);
-void do_died(subuser *user);
+void do_died(subuser *user, acetables *g_ape);
void check_timeout(acetables *g_ape, int *last);
void grant_aceop(USERS *user);

0 comments on commit a5ab6f8

Please sign in to comment.