From 561891aac5f99688855ed8298259184328cfeef7 Mon Sep 17 00:00:00 2001 From: Johan Wikman Date: Mon, 21 Sep 2015 13:41:22 +0300 Subject: [PATCH 1/4] Fix for double free, possible fix for bug601. In the end of execute_sescmd_in_backend the buffer was consumed in case the protocol function failed. Or actually if it returned something else but 1. In the case of mysql_backend, the buffer is always freed when authorizing and either consumed or placed on the dcb writequeue when the data is written. That is, it is never ok to consume the buffer in this function. The end-result is likely to be an abort. --- server/modules/routing/readwritesplit/readwritesplit.c | 1 - 1 file changed, 1 deletion(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 42b64a34c..9a322d1f3 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -4243,7 +4243,6 @@ static bool execute_sescmd_in_backend( } else { - while((buf = GWBUF_CONSUME_ALL(buf)) != NULL); succp = false; } return_succp: From 988a8d7008009a36a394df71ab84a11bbaa970f5 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Thu, 17 Sep 2015 07:13:24 +0300 Subject: [PATCH 2/4] Fixed error messages not being printed and cleaned up the function. --- server/modules/protocol/mysql_client.c | 300 +++++++++++++------------ 1 file changed, 161 insertions(+), 139 deletions(-) diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 87b812c06..8219c7119 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1331,164 +1331,186 @@ return_1: } /** - * set listener for mysql protocol, retur 1 on success and 0 in failure + * Bind the DCB to a network port or a UNIX Domain Socket. + * @param listen_dcb Listener DCB + * @param config_bind Bind address in either IP:PORT format for network sockets or PATH for UNIX Domain Sockets + * @return 1 on success, 0 on error */ -int gw_MySQLListener( - DCB *listen_dcb, - char *config_bind) +int gw_MySQLListener(DCB *listen_dcb, + char *config_bind) { - int l_so; - int syseno = 0; - struct sockaddr_in serv_addr; - struct sockaddr_un local_addr; - struct sockaddr *current_addr; - int one = 1; - int rc; - bool is_tcp = false; - memset(&serv_addr,0,sizeof(serv_addr)); - memset(&local_addr,0,sizeof(local_addr)); + int l_so; + struct sockaddr_in serv_addr; + struct sockaddr_un local_addr; + struct sockaddr *current_addr; + int one = 1; + int rc; + bool is_tcp = false; + memset(&serv_addr, 0, sizeof (serv_addr)); + memset(&local_addr, 0, sizeof (local_addr)); - if (strchr(config_bind, '/')) { - char *tmp = strrchr(config_bind, ':'); - if (tmp) - *tmp = '\0'; + if (strchr(config_bind, '/')) + { + char *tmp = strrchr(config_bind, ':'); + if (tmp) + *tmp = '\0'; - // UNIX socket create - if ((l_so = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) { - char errbuf[STRERROR_BUFLEN]; - fprintf(stderr, - "\n* Error: can't create UNIX socket due " - "error %i, %s.\n\n\t", - errno, - strerror_r(errno, errbuf, sizeof(errbuf))); - return 0; - } - memset(&local_addr, 0, sizeof(local_addr)); - local_addr.sun_family = AF_UNIX; - strncpy(local_addr.sun_path, config_bind, sizeof(local_addr.sun_path) - 1); - - current_addr = (struct sockaddr *) &local_addr; - - } else { - /* MaxScale, as default, will bind on port 4406 */ - if (!parse_bindconfig(config_bind, 4406, &serv_addr)) { - fprintf(stderr, "Error in parse_bindconfig for [%s]\n", config_bind); - return 0; - } - // TCP socket create - if ((l_so = socket(AF_INET, SOCK_STREAM, 0)) < 0) { - char errbuf[STRERROR_BUFLEN]; - fprintf(stderr, - "\n* Error: can't create socket due " - "error %i, %s.\n\n\t", - errno, - strerror_r(errno, errbuf, sizeof(errbuf))); - return 0; - } - - current_addr = (struct sockaddr *) &serv_addr; - is_tcp = true; - } - - listen_dcb->fd = -1; - - // socket options - if((syseno = setsockopt(l_so, SOL_SOCKET, SO_REUSEADDR, (char *)&one, sizeof(one))) != 0){ - char errbuf[STRERROR_BUFLEN]; - LOGIF(LE, (skygw_log_write_flush(LOGFILE_ERROR,"Error: Failed to set socket options. Error %d: %s", errno, strerror_r(errno, errbuf, sizeof(errbuf))))); - } - - if(is_tcp) - { + // UNIX socket create + if ((l_so = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) + { char errbuf[STRERROR_BUFLEN]; - if((syseno = setsockopt(l_so, IPPROTO_TCP, TCP_NODELAY, (char *)&one, sizeof(one))) != 0){ - LOGIF(LE, (skygw_log_write_flush(LOGFILE_ERROR,"Error: Failed to set socket options. Error %d: %s", errno, strerror_r(errno, errbuf, sizeof(errbuf))))); - } - } - // set NONBLOCKING mode - setnonblocking(l_so); + skygw_log_write(LE, + "Error: Can't create UNIX socket: %i, %s", + errno, + strerror_r(errno, errbuf, sizeof (errbuf))); + return 0; + } + memset(&local_addr, 0, sizeof (local_addr)); + local_addr.sun_family = AF_UNIX; + strncpy(local_addr.sun_path, config_bind, sizeof (local_addr.sun_path) - 1); - /* get the right socket family for bind */ - switch (current_addr->sa_family) { - case AF_UNIX: - rc = unlink(config_bind); - if ( (rc == -1) && (errno!=ENOENT) ) { - fprintf(stderr, "Error unlink Unix Socket %s\n", config_bind); - } + current_addr = (struct sockaddr *) &local_addr; - if (bind(l_so, (struct sockaddr *) &local_addr, sizeof(local_addr)) < 0) { - char errbuf[STRERROR_BUFLEN]; - fprintf(stderr, - "\n* Bind failed due error %i, %s.\n", - errno, - strerror_r(errno, errbuf, sizeof(errbuf))); - fprintf(stderr, "* Can't bind to %s\n\n", config_bind); - close(l_so); - return 0; - } + } + else + { + /* This is partially dead code, MaxScale will never start without explicit + * ports defined for all listeners. Thus the default port is never used. + */ + if (!parse_bindconfig(config_bind, 4406, &serv_addr)) + { + skygw_log_write(LE, "Error in parse_bindconfig for [%s]", config_bind); + return 0; + } - /* set permission for all users */ - if (chmod(config_bind, 0777) < 0) { - char errbuf[STRERROR_BUFLEN]; - fprintf(stderr, - "\n* chmod failed for %s due error %i, %s.\n\n", - config_bind, - errno, - strerror_r(errno, errbuf, sizeof(errbuf))); - } + /** Create the TCP socket */ + if ((l_so = socket(AF_INET, SOCK_STREAM, 0)) < 0) + { + char errbuf[STRERROR_BUFLEN]; + skygw_log_write(LE, + "Error: Can't create socket: %i, %s", + errno, + strerror_r(errno, errbuf, sizeof (errbuf))); + return 0; + } - break; + current_addr = (struct sockaddr *) &serv_addr; + is_tcp = true; + } - case AF_INET: - if (bind(l_so, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0) { - char errbuf[STRERROR_BUFLEN]; - fprintf(stderr, - "\n* Bind failed due error %i, %s.\n", - errno, - strerror_r(errno, errbuf, sizeof(errbuf))); - fprintf(stderr, "* Can't bind to %s\n\n", config_bind); - close(l_so); - return 0; - } - break; + listen_dcb->fd = -1; - default: - fprintf(stderr, "* Socket Family %i not supported\n", current_addr->sa_family); - close(l_so); - return 0; - } + // socket options + if (setsockopt(l_so, SOL_SOCKET, SO_REUSEADDR, (char *) &one, sizeof (one)) != 0) + { + char errbuf[STRERROR_BUFLEN]; + LOGIF(LE, (skygw_log_write_flush(LOGFILE_ERROR, "Error: Failed to set socket options. Error %d: %s", errno, strerror_r(errno, errbuf, sizeof (errbuf))))); + } - rc = listen(l_so, 10 * SOMAXCONN); + if (is_tcp) + { + char errbuf[STRERROR_BUFLEN]; + if (setsockopt(l_so, IPPROTO_TCP, TCP_NODELAY, (char *) &one, sizeof (one)) != 0) + { + LOGIF(LE, (skygw_log_write_flush(LOGFILE_ERROR, "Error: Failed to set socket options. Error %d: %s", errno, strerror_r(errno, errbuf, sizeof (errbuf))))); + } + } + // set NONBLOCKING mode + if (setnonblocking(l_so) != 0) + { + skygw_log_write(LE, "Error: Failed to set socket to non-blocking mode."); + close(l_so); + return 0; + } - if (rc == 0) { - LOGIF(LM, (skygw_log_write_flush(LOGFILE_MESSAGE,"Listening MySQL connections at %s", config_bind))); - } else { - int eno = errno; - errno = 0; + /* get the right socket family for bind */ + switch (current_addr->sa_family) + { + case AF_UNIX: + rc = unlink(config_bind); + if ((rc == -1) && (errno != ENOENT)) + { char errbuf[STRERROR_BUFLEN]; - fprintf(stderr, - "\n* Failed to start listening MySQL due error %d, %s\n\n", - eno, - strerror_r(eno, errbuf, sizeof(errbuf))); - close(l_so); + skygw_log_write(LE, "Error: Failed to unlink Unix Socket %s: %d %s", + config_bind, errno, strerror_r(errno, errbuf, sizeof (errbuf))); + } + + if (bind(l_so, (struct sockaddr *) &local_addr, sizeof (local_addr)) < 0) + { + char errbuf[STRERROR_BUFLEN]; + skygw_log_write(LE, + "Error: Failed to bind to UNIX Domain socket '%s': %i, %s", + config_bind, + errno, + strerror_r(errno, errbuf, sizeof (errbuf))); + close(l_so); return 0; - } - // assign l_so to dcb - listen_dcb->fd = l_so; + } - // add listening socket to poll structure - if (poll_add_dcb(listen_dcb) == -1) { - fprintf(stderr, - "\n* MaxScale encountered system limit while " - "attempting to register on an epoll instance.\n\n"); - return 0; - } + /* set permission for all users */ + if (chmod(config_bind, 0777) < 0) + { + char errbuf[STRERROR_BUFLEN]; + skygw_log_write(LE, + "Error: Failed to change permissions on UNIX Domain socket '%s': %i, %s", + config_bind, + errno, + strerror_r(errno, errbuf, sizeof (errbuf))); + } + + break; + + case AF_INET: + if (bind(l_so, (struct sockaddr *) &serv_addr, sizeof (serv_addr)) < 0) + { + char errbuf[STRERROR_BUFLEN]; + skygw_log_write(LE, + "Error: Failed to bind on '%s': %i, %s", + config_bind, + errno, + strerror_r(errno, errbuf, sizeof (errbuf))); + close(l_so); + return 0; + } + break; + + default: + skygw_log_write(LE, "Error: Socket Family %i not supported\n", current_addr->sa_family); + close(l_so); + return 0; + } + + if (listen(l_so, 10 * SOMAXCONN) != 0) + { + char errbuf[STRERROR_BUFLEN]; + skygw_log_write(LE, + "Failed to start listening on '%s': %d, %s", + config_bind, + errno, + strerror_r(errno, errbuf, sizeof (errbuf))); + close(l_so); + return 0; + } + + LOGIF(LM, (skygw_log_write_flush(LOGFILE_MESSAGE, "Listening MySQL connections at %s", config_bind))); + + // assign l_so to dcb + listen_dcb->fd = l_so; + + // add listening socket to poll structure + if (poll_add_dcb(listen_dcb) != 0) + { + skygw_log_write(LE, + "MaxScale encountered system limit while " + "attempting to register on an epoll instance."); + return 0; + } #if defined(FAKE_CODE) - conn_open[l_so] = true; + conn_open[l_so] = true; #endif /* FAKE_CODE */ - listen_dcb->func.accept = gw_MySQLAccept; + listen_dcb->func.accept = gw_MySQLAccept; - return 1; + return 1; } From 11c8ef5b9251601537634be673019441253a3365 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 21 Sep 2015 13:56:56 +0300 Subject: [PATCH 3/4] Cleaned up code based on the review of the code. --- server/modules/protocol/mysql_client.c | 40 +++++++++++++++----------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 8219c7119..372a42f38 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1346,8 +1346,8 @@ int gw_MySQLListener(DCB *listen_dcb, int one = 1; int rc; bool is_tcp = false; - memset(&serv_addr, 0, sizeof (serv_addr)); - memset(&local_addr, 0, sizeof (local_addr)); + memset(&serv_addr, 0, sizeof(serv_addr)); + memset(&local_addr, 0, sizeof(local_addr)); if (strchr(config_bind, '/')) { @@ -1362,12 +1362,12 @@ int gw_MySQLListener(DCB *listen_dcb, skygw_log_write(LE, "Error: Can't create UNIX socket: %i, %s", errno, - strerror_r(errno, errbuf, sizeof (errbuf))); + strerror_r(errno, errbuf, sizeof(errbuf))); return 0; } - memset(&local_addr, 0, sizeof (local_addr)); + memset(&local_addr, 0, sizeof(local_addr)); local_addr.sun_family = AF_UNIX; - strncpy(local_addr.sun_path, config_bind, sizeof (local_addr.sun_path) - 1); + strncpy(local_addr.sun_path, config_bind, sizeof(local_addr.sun_path) - 1); current_addr = (struct sockaddr *) &local_addr; @@ -1390,7 +1390,7 @@ int gw_MySQLListener(DCB *listen_dcb, skygw_log_write(LE, "Error: Can't create socket: %i, %s", errno, - strerror_r(errno, errbuf, sizeof (errbuf))); + strerror_r(errno, errbuf, sizeof(errbuf))); return 0; } @@ -1401,18 +1401,24 @@ int gw_MySQLListener(DCB *listen_dcb, listen_dcb->fd = -1; // socket options - if (setsockopt(l_so, SOL_SOCKET, SO_REUSEADDR, (char *) &one, sizeof (one)) != 0) + if (setsockopt(l_so, SOL_SOCKET, SO_REUSEADDR, (char *) &one, sizeof(one)) != 0) { char errbuf[STRERROR_BUFLEN]; - LOGIF(LE, (skygw_log_write_flush(LOGFILE_ERROR, "Error: Failed to set socket options. Error %d: %s", errno, strerror_r(errno, errbuf, sizeof (errbuf))))); + LOGIF(LE, (skygw_log_write_flush(LOGFILE_ERROR, + "Error: Failed to set socket options. Error %d: %s", + errno, + strerror_r(errno, errbuf, sizeof(errbuf))))); } if (is_tcp) { char errbuf[STRERROR_BUFLEN]; - if (setsockopt(l_so, IPPROTO_TCP, TCP_NODELAY, (char *) &one, sizeof (one)) != 0) + if (setsockopt(l_so, IPPROTO_TCP, TCP_NODELAY, (char *) &one, sizeof(one)) != 0) { - LOGIF(LE, (skygw_log_write_flush(LOGFILE_ERROR, "Error: Failed to set socket options. Error %d: %s", errno, strerror_r(errno, errbuf, sizeof (errbuf))))); + LOGIF(LE, (skygw_log_write_flush(LOGFILE_ERROR, + "Error: Failed to set socket options. Error %d: %s", + errno, + strerror_r(errno, errbuf, sizeof(errbuf))))); } } // set NONBLOCKING mode @@ -1432,17 +1438,17 @@ int gw_MySQLListener(DCB *listen_dcb, { char errbuf[STRERROR_BUFLEN]; skygw_log_write(LE, "Error: Failed to unlink Unix Socket %s: %d %s", - config_bind, errno, strerror_r(errno, errbuf, sizeof (errbuf))); + config_bind, errno, strerror_r(errno, errbuf, sizeof(errbuf))); } - if (bind(l_so, (struct sockaddr *) &local_addr, sizeof (local_addr)) < 0) + if (bind(l_so, (struct sockaddr *) &local_addr, sizeof(local_addr)) < 0) { char errbuf[STRERROR_BUFLEN]; skygw_log_write(LE, "Error: Failed to bind to UNIX Domain socket '%s': %i, %s", config_bind, errno, - strerror_r(errno, errbuf, sizeof (errbuf))); + strerror_r(errno, errbuf, sizeof(errbuf))); close(l_so); return 0; } @@ -1455,20 +1461,20 @@ int gw_MySQLListener(DCB *listen_dcb, "Error: Failed to change permissions on UNIX Domain socket '%s': %i, %s", config_bind, errno, - strerror_r(errno, errbuf, sizeof (errbuf))); + strerror_r(errno, errbuf, sizeof(errbuf))); } break; case AF_INET: - if (bind(l_so, (struct sockaddr *) &serv_addr, sizeof (serv_addr)) < 0) + if (bind(l_so, (struct sockaddr *) &serv_addr, sizeof(serv_addr)) < 0) { char errbuf[STRERROR_BUFLEN]; skygw_log_write(LE, "Error: Failed to bind on '%s': %i, %s", config_bind, errno, - strerror_r(errno, errbuf, sizeof (errbuf))); + strerror_r(errno, errbuf, sizeof(errbuf))); close(l_so); return 0; } @@ -1487,7 +1493,7 @@ int gw_MySQLListener(DCB *listen_dcb, "Failed to start listening on '%s': %d, %s", config_bind, errno, - strerror_r(errno, errbuf, sizeof (errbuf))); + strerror_r(errno, errbuf, sizeof(errbuf))); close(l_so); return 0; } From 338b870cd1aa92c653b90fe9e1665d83101e4c86 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Mon, 21 Sep 2015 17:07:37 +0300 Subject: [PATCH 4/4] Fix to MXS-373: https://mariadb.atlassian.net/browse/MXS-373 The log manager is initialized only once and skygw_log_sync_all now checks if the log manager has been successfully started before interacting with the log manager --- log_manager/log_manager.cc | 23 +++++++++++++++++++---- server/core/gateway.c | 1 - 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/log_manager/log_manager.cc b/log_manager/log_manager.cc index fd2a74195..157c9ea04 100644 --- a/log_manager/log_manager.cc +++ b/log_manager/log_manager.cc @@ -109,6 +109,10 @@ static bool flushall_done_flag; static int default_log_augmentation = 0; static int log_augmentation = default_log_augmentation; +/** This is used to detect if the initialization of the log manager has failed + * and that it isn't initialized again after a failure has occurred. */ +static bool fatal_error = false; + /** Writer thread structure */ struct filewriter_st { #if defined(SS_DEBUG) @@ -1681,7 +1685,7 @@ static bool logmanager_register( * and its members which would probabaly lead to NULL pointer * reference. */ - if (!writep) { + if (!writep || fatal_error) { succp = false; goto return_succp; } @@ -1709,6 +1713,11 @@ static bool logmanager_register( } return_succp: + + if(!succp) + { + fatal_error = true; + } release_lock(&lmlock); return succp; } @@ -3232,9 +3241,15 @@ void flushall_logfiles(bool flush) void skygw_log_sync_all(void) { if(!use_stdout)skygw_log_write(LOGFILE_TRACE,"Starting log flushing to disk."); - flushall_logfiles(true); - skygw_message_send(lm->lm_logmes); - skygw_message_wait(lm->lm_clientmes); + + /** If initialization of the log manager has not been done, lm pointer can be + * NULL. */ + if(lm) + { + flushall_logfiles(true); + skygw_message_send(lm->lm_logmes); + skygw_message_wait(lm->lm_clientmes); + } } /** diff --git a/server/core/gateway.c b/server/core/gateway.c index 5c2355bee..0907d6365 100644 --- a/server/core/gateway.c +++ b/server/core/gateway.c @@ -1648,7 +1648,6 @@ int main(int argc, char **argv) if (!resolve_maxscale_conf_fname(&cnf_file_path, pathbuf, cnf_file_arg)) { - ss_dassert(cnf_file_path == NULL); rc = MAXSCALE_BADCONFIG; goto return_main; }