hcq/MaxScale
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Partial fix to #463, http://bugs.skysql.com/show_bug.cgi?id=463

Browse Source 
log_manager.cc: fixed block buffer overflow. Queries are logged to trace log and long queries exceed the bufsize in length. Those were written beyond allocated memory areas.
mysql_client_server_protocol.h: added mysql_protocol_state_t to indicate whether MySQL protocol object is allocated, usable or freed. Freed means that memory allocations made by the protocol are freed. That is, command history etc.
mysql_backend.c: gw_backend_hangup and gw_error_backend_event used to call error handling function although session was already closing. Added check for session state.
mysql_client.c: route_by_statement lost some packets in case where query was sent in multiple packets.
mysql_common.c: gw_MySQL_get_next_packet failed in packet handling with route_by_statement. When multi-packet query was merged into one, packet type wasn't copied.
	protocol_archive_srv_command and mysql_protocol_done didn't have proper locking in place which lead to occasional crashes.
This commit is contained in:
VilhoRaatikka
2014-08-14 13:05:05 +03:00
parent 97ab902ede
commit d3a79ce7c4
7 changed files with 137 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
log_manager/log_manager.cc
View File

@ -255,11 +255,7 @@ static int logmanager_write_log(
static blockbuf_t* blockbuf_init(logfile_id_t id); static blockbuf_t* blockbuf_init(logfile_id_t id);
static void blockbuf_node_done(void* bb_data); static void blockbuf_node_done(void* bb_data);
static char* blockbuf_get_writepos( static char* blockbuf_get_writepos(
#if 0
int** refcount,
#else
blockbuf_t** p_bb, blockbuf_t** p_bb,
#endif
logfile_id_t id, logfile_id_t id,
size_t str_len, size_t str_len,
bool flush); bool flush);
@ -653,7 +649,16 @@ static int logmanager_write_log(
int safe_str_len; int safe_str_len;
timestamp_len = get_timestamp_len(); timestamp_len = get_timestamp_len();
safe_str_len = MIN(timestamp_len-1+str_len, lf->lf_buf_size);
/** Findout how much can be safely written with current block size */
if (timestamp_len-1+str_len > lf->lf_buf_size)
{
safe_str_len = lf->lf_buf_size;
}
else
{
safe_str_len = timestamp_len-1+str_len;
}
/** /**
* Seek write position and register to block buffer. * Seek write position and register to block buffer.
* Then print formatted string to write position. * Then print formatted string to write position.
@ -673,9 +678,9 @@ static int logmanager_write_log(
* of the timestamp string. * of the timestamp string.
*/ */
if (use_valist) { if (use_valist) {
vsnprintf(wp+timestamp_len, safe_str_len, str, valist); vsnprintf(wp+timestamp_len, safe_str_len-timestamp_len, str, valist);
} else { } else {
snprintf(wp+timestamp_len, safe_str_len, "%s", str); snprintf(wp+timestamp_len, safe_str_len-timestamp_len, "%s", str);
} }
/** write to syslog */ /** write to syslog */
@ -694,12 +699,7 @@ static int logmanager_write_log(
break; break;
} }
} }
wp[safe_str_len-1] = '\n';
/** remove double line feed */
if (wp[timestamp_len+str_len-2] == '\n') {
wp[timestamp_len+str_len-2]=' ';
}
wp[timestamp_len+str_len-1]='\n';
blockbuf_unregister(bb); blockbuf_unregister(bb);
/** /**

4
server/include/dcb.h
View File

@ -145,9 +145,9 @@ typedef enum {
DCB_STATE_POLLING, /*< Waiting in the poll loop */ DCB_STATE_POLLING, /*< Waiting in the poll loop */
DCB_STATE_LISTENING, /*< The DCB is for a listening socket */ DCB_STATE_LISTENING, /*< The DCB is for a listening socket */
DCB_STATE_DISCONNECTED, /*< The socket is now closed */ DCB_STATE_DISCONNECTED, /*< The socket is now closed */
DCB_STATE_FREED, /*< Memory freed */
DCB_STATE_NOPOLLING, /*< Removed from poll mask */ DCB_STATE_NOPOLLING, /*< Removed from poll mask */
DCB_STATE_ZOMBIE /*< DCB is no longer active, waiting to free it */ DCB_STATE_ZOMBIE, /*< DCB is no longer active, waiting to free it */
DCB_STATE_FREED /*< Memory freed */
} dcb_state_t; } dcb_state_t;
typedef enum { typedef enum {

7
server/modules/include/mysql_client_server_protocol.h
View File

@ -102,6 +102,12 @@ typedef enum {
MYSQL_IDLE MYSQL_IDLE
} mysql_auth_state_t; } mysql_auth_state_t;
typedef enum {
MYSQL_PROTOCOL_ALLOC,
MYSQL_PROTOCOL_ACTIVE,
MYSQL_PROTOCOL_DONE
} mysql_protocol_state_t;
/* /*
* MySQL session specific data * MySQL session specific data
@ -270,6 +276,7 @@ typedef struct {
server_command_t protocol_command; /*< session command list */ server_command_t protocol_command; /*< session command list */
server_command_t* protocol_cmd_history; /*< session command history */ server_command_t* protocol_cmd_history; /*< session command history */
mysql_auth_state_t protocol_auth_state; /*< Authentication status */ mysql_auth_state_t protocol_auth_state; /*< Authentication status */
mysql_protocol_state_t protocol_state; /*< Protocol struct status */
uint8_t scramble[MYSQL_SCRAMBLE_LEN]; /*< server scramble, uint8_t scramble[MYSQL_SCRAMBLE_LEN]; /*< server scramble,
* created or received */ * created or received */
uint32_t server_capabilities; /*< server capabilities, uint32_t server_capabilities; /*< server capabilities,

77
server/modules/protocol/mysql_backend.c
View File

@ -762,12 +762,13 @@ return_rc:
*/ */
static int gw_error_backend_event(DCB *dcb) static int gw_error_backend_event(DCB *dcb)
{ {
SESSION* session; SESSION* session;
void* rsession; void* rsession;
ROUTER_OBJECT* router; ROUTER_OBJECT* router;
ROUTER* router_instance; ROUTER* router_instance;
GWBUF* errbuf; GWBUF* errbuf;
bool succp; bool succp;
session_state_t ses_state;
CHK_DCB(dcb); CHK_DCB(dcb);
session = dcb->session; session = dcb->session;
@ -787,7 +788,7 @@ static int gw_error_backend_event(DCB *dcb)
* closed by router and COM_QUIT sent or there was an error which * closed by router and COM_QUIT sent or there was an error which
* have already been handled. * have already been handled.
*/ */
if (dcb->session != DCB_STATE_POLLING) if (dcb->state != DCB_STATE_POLLING)
{ {
return 1; return 1;
} }
@ -796,6 +797,29 @@ static int gw_error_backend_event(DCB *dcb)
0, 0,
"Lost connection to backend server."); "Lost connection to backend server.");
spinlock_acquire(&session->ses_lock);
ses_state = session->state;
spinlock_release(&session->ses_lock);
/**
* Session might be initialized when DCB already is in the poll set.
* Thus hangup can occur in the middle of session initialization.
* Only complete and successfully initialized sessions allow for
* calling error handler.
*/
while (ses_state == SESSION_STATE_READY)
{
spinlock_acquire(&session->ses_lock);
ses_state = session->state;
spinlock_release(&session->ses_lock);
}
if (ses_state != SESSION_STATE_ROUTER_READY)
{
gwbuf_free(errbuf);
goto retblock;
}
router->handleError(router_instance, router->handleError(router_instance,
rsession, rsession,
errbuf, errbuf,
@ -811,6 +835,7 @@ static int gw_error_backend_event(DCB *dcb)
} }
dcb_close(dcb); dcb_close(dcb);
retblock:
return 1; return 1;
} }
@ -924,12 +949,13 @@ return_fd:
static int static int
gw_backend_hangup(DCB *dcb) gw_backend_hangup(DCB *dcb)
{ {
SESSION* session; SESSION* session;
void* rsession; void* rsession;
ROUTER_OBJECT* router; ROUTER_OBJECT* router;
ROUTER* router_instance; ROUTER* router_instance;
bool succp; bool succp;
GWBUF* errbuf; GWBUF* errbuf;
session_state_t ses_state;
CHK_DCB(dcb); CHK_DCB(dcb);
session = dcb->session; session = dcb->session;
@ -951,6 +977,28 @@ gw_backend_hangup(DCB *dcb)
0, 0,
"Lost connection to backend server."); "Lost connection to backend server.");
spinlock_acquire(&session->ses_lock);
ses_state = session->state;
spinlock_release(&session->ses_lock);
/**
* Session might be initialized when DCB already is in the poll set.
* Thus hangup can occur in the middle of session initialization.
* Only complete and successfully initialized sessions allow for
* calling error handler.
*/
while (ses_state == SESSION_STATE_READY)
{
spinlock_acquire(&session->ses_lock);
ses_state = session->state;
spinlock_release(&session->ses_lock);
}
if (ses_state != SESSION_STATE_ROUTER_READY)
{
gwbuf_free(errbuf);
goto retblock;
}
router->handleError(router_instance, router->handleError(router_instance,
rsession, rsession,
errbuf, errbuf,
@ -972,7 +1020,8 @@ gw_backend_hangup(DCB *dcb)
} }
dcb_close(dcb); dcb_close(dcb);
return 1; retblock:
return 1;
} }
/** /**

18
server/modules/protocol/mysql_client.c
View File

@ -65,7 +65,7 @@ static int gw_client_hangup_event(DCB *dcb);
int mysql_send_ok(DCB *dcb, int packet_number, int in_affected_rows, const char* mysql_message); int mysql_send_ok(DCB *dcb, int packet_number, int in_affected_rows, const char* mysql_message);
int MySQLSendHandshake(DCB* dcb); int MySQLSendHandshake(DCB* dcb);
static int gw_mysql_do_authentication(DCB *dcb, GWBUF *queue); static int gw_mysql_do_authentication(DCB *dcb, GWBUF *queue);
static int route_by_statement(SESSION *, GWBUF *); static int route_by_statement(SESSION *, GWBUF **);
/* /*
* The "module object" for the mysqld client protocol module. * The "module object" for the mysqld client protocol module.
@ -783,7 +783,7 @@ int gw_read_client_event(
* Feed each statement completely and separately * Feed each statement completely and separately
* to router. * to router.
*/ */
rc = route_by_statement(session, read_buffer); rc = route_by_statement(session, &read_buffer);
if (read_buffer != NULL) if (read_buffer != NULL)
{ {
@ -1383,7 +1383,7 @@ gw_client_hangup_event(DCB *dcb)
#if defined(SS_DEBUG) #if defined(SS_DEBUG)
LOGIF(LE, (skygw_log_write_flush( LOGIF(LE, (skygw_log_write_flush(
LOGFILE_ERROR, LOGFILE_ERROR,
"Client hangup error handling."))); "Client hangup error handling.")));
#endif #endif
dcb_close(dcb); dcb_close(dcb);
return 1; return 1;
@ -1399,7 +1399,9 @@ gw_client_hangup_event(DCB *dcb)
* Return 1 in success. If the last packet is incomplete return success but * Return 1 in success. If the last packet is incomplete return success but
* leave incomplete packet to readbuf. * leave incomplete packet to readbuf.
*/ */
static int route_by_statement(SESSION *session, GWBUF *readbuf) static int route_by_statement(
SESSION* session,
GWBUF** p_readbuf)
{ {
int rc = -1; int rc = -1;
GWBUF* packetbuf; GWBUF* packetbuf;
@ -1407,7 +1409,7 @@ static int route_by_statement(SESSION *session, GWBUF *readbuf)
gwbuf_type_t prevtype; gwbuf_type_t prevtype;
GWBUF* tmpbuf; GWBUF* tmpbuf;
tmpbuf = readbuf; tmpbuf = *p_readbuf;
while (tmpbuf != NULL) while (tmpbuf != NULL)
{ {
ss_dassert(GWBUF_IS_TYPE_MYSQL(tmpbuf)); ss_dassert(GWBUF_IS_TYPE_MYSQL(tmpbuf));
@ -1416,9 +1418,9 @@ static int route_by_statement(SESSION *session, GWBUF *readbuf)
#endif #endif
do do
{ {
ss_dassert(GWBUF_IS_TYPE_MYSQL(readbuf)); ss_dassert(GWBUF_IS_TYPE_MYSQL((*p_readbuf)));
packetbuf = gw_MySQL_get_next_packet(&readbuf); packetbuf = gw_MySQL_get_next_packet(p_readbuf);
ss_dassert(GWBUF_IS_TYPE_MYSQL(packetbuf)); ss_dassert(GWBUF_IS_TYPE_MYSQL(packetbuf));
@ -1447,7 +1449,7 @@ static int route_by_statement(SESSION *session, GWBUF *readbuf)
goto return_rc; goto return_rc;
} }
} }
while (readbuf != NULL); while (*p_readbuf != NULL);
return_rc: return_rc:
return rc; return rc;

46
server/modules/protocol/mysql_common.c
View File

@ -79,6 +79,7 @@ MySQLProtocol* mysql_protocol_init(
strerror(eno)))); strerror(eno))));
goto return_p; goto return_p;
} }
p->protocol_state = MYSQL_PROTOCOL_ALLOC;
p->protocol_auth_state = MYSQL_ALLOC; p->protocol_auth_state = MYSQL_ALLOC;
p->protocol_command.scom_cmd = MYSQL_COM_UNDEFINED; p->protocol_command.scom_cmd = MYSQL_COM_UNDEFINED;
p->protocol_command.scom_nresponse_packets = 0; p->protocol_command.scom_nresponse_packets = 0;
@ -90,6 +91,7 @@ MySQLProtocol* mysql_protocol_init(
/*< Assign fd with protocol */ /*< Assign fd with protocol */
p->fd = fd; p->fd = fd;
p->owner_dcb = dcb; p->owner_dcb = dcb;
p->protocol_state = MYSQL_PROTOCOL_ACTIVE;
CHK_PROTOCOL(p); CHK_PROTOCOL(p);
return_p: return_p:
return p; return p;
@ -107,15 +109,25 @@ return_p:
void mysql_protocol_done ( void mysql_protocol_done (
DCB* dcb) DCB* dcb)
{ {
server_command_t* scmd = ((MySQLProtocol *)dcb->protocol)->protocol_cmd_history; MySQLProtocol* p;
server_command_t* scmd;
server_command_t* scmd2; server_command_t* scmd2;
p = (MySQLProtocol *)dcb->protocol;
spinlock_acquire(&p->protocol_lock);
scmd = p->protocol_cmd_history;
while (scmd != NULL) while (scmd != NULL)
{ {
scmd2 = scmd->scom_next; scmd2 = scmd->scom_next;
free(scmd); free(scmd);
scmd = scmd2; scmd = scmd2;
} }
p->protocol_state = MYSQL_PROTOCOL_DONE;
spinlock_release(&p->protocol_lock);
} }
@ -1542,6 +1554,7 @@ GWBUF* gw_MySQL_get_next_packet(
packetbuf = gwbuf_alloc(packetlen); packetbuf = gwbuf_alloc(packetlen);
target = GWBUF_DATA(packetbuf); target = GWBUF_DATA(packetbuf);
packetbuf->gwbuf_type = readbuf->gwbuf_type; /*< Copy the type too */
while (nbytes_copied < packetlen) while (nbytes_copied < packetlen)
{ {
@ -1549,9 +1562,10 @@ GWBUF* gw_MySQL_get_next_packet(
size_t buflen = GWBUF_LENGTH(readbuf); size_t buflen = GWBUF_LENGTH(readbuf);
memcpy(target+nbytes_copied, src, buflen); memcpy(target+nbytes_copied, src, buflen);
*p_readbuf = gwbuf_consume(readbuf, buflen); readbuf = gwbuf_consume(readbuf, buflen);
nbytes_copied += buflen; nbytes_copied += buflen;
} }
*p_readbuf = readbuf;
ss_dassert(nbytes_copied == packetlen); ss_dassert(nbytes_copied == packetlen);
} }
else else
@ -1625,11 +1639,16 @@ void protocol_archive_srv_command(
MySQLProtocol* p) MySQLProtocol* p)
{ {
server_command_t* s1; server_command_t* s1;
server_command_t** s2; server_command_t* h1;
int len = 0; int len = 0;
spinlock_acquire(&p->protocol_lock); spinlock_acquire(&p->protocol_lock);
if (p->protocol_state != MYSQL_PROTOCOL_ACTIVE)
{
goto retblock;
}
s1 = &p->protocol_command; s1 = &p->protocol_command;
LOGIF(LT, (skygw_log_write( LOGIF(LT, (skygw_log_write(
@ -1639,17 +1658,18 @@ void protocol_archive_srv_command(
p->owner_dcb->fd))); p->owner_dcb->fd)));
/** Copy to history list */ /** Copy to history list */
s2 = &p->protocol_cmd_history; if ((h1 = p->protocol_cmd_history) == NULL)
if (*s2 != NULL)
{ {
while ((*s2)->scom_next != NULL) p->protocol_cmd_history = server_command_copy(s1);
{ }
*s2 = (*s2)->scom_next; else
len += 1; {
} while (h1->scom_next != NULL)
{
h1 = h1->scom_next;
}
h1->scom_next = server_command_copy(s1);
} }
*s2 = server_command_copy(s1);
/** Keep history limits, remove oldest */ /** Keep history limits, remove oldest */
if (len > MAX_CMD_HISTORY) if (len > MAX_CMD_HISTORY)
@ -1669,6 +1689,8 @@ void protocol_archive_srv_command(
p->protocol_command = *(s1->scom_next); p->protocol_command = *(s1->scom_next);
free(s1->scom_next); free(s1->scom_next);
} }
retblock:
spinlock_release(&p->protocol_lock); spinlock_release(&p->protocol_lock);
} }

2
server/modules/routing/readwritesplit/readwritesplit.c
View File

@ -1549,7 +1549,7 @@ static void clientReply (
size_t len = MYSQL_GET_PACKET_LEN(buf); size_t len = MYSQL_GET_PACKET_LEN(buf);
char* cmdstr = (char *)malloc(len+1); char* cmdstr = (char *)malloc(len+1);
snprintf(cmdstr, len+1, "%s", &buf[5]); snprintf(cmdstr, len, "%s", &buf[5]);
LOGIF(LE, (skygw_log_write_flush( LOGIF(LE, (skygw_log_write_flush(
LOGFILE_ERROR, LOGFILE_ERROR,