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

Changes to try to eliminate setting dcb->session to NULL with risk of crashing system.

Browse Source
This commit is contained in:
counterpoint
2015-08-24 12:12:43 +01:00
parent 5e2e2585ad
commit 37ac158791
7 changed files with 182 additions and 194 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
server/core/poll.c
View File

@ -26,7 +26,6 @@
#include <poll.h> #include <poll.h>
#include <dcb.h> #include <dcb.h>
#include <atomic.h> #include <atomic.h>
#include <session.h>
#include <gwbitmask.h> #include <gwbitmask.h>
#include <skygw_utils.h> #include <skygw_utils.h>
#include <log_manager.h> #include <log_manager.h>
@ -909,14 +908,6 @@ unsigned long qtime;
{ {
if (dcb->state == DCB_STATE_LISTENING) if (dcb->state == DCB_STATE_LISTENING)
{ {
if (NULL == dcb->session)
{
dcb->session = (SESSION *)session_alloc(dcb->service, dcb);
if (dcb->session)
{
dcb->session->state = SESSION_STATE_LISTENER;
}
}
LOGIF(LD, (skygw_log_write( LOGIF(LD, (skygw_log_write(
LOGFILE_DEBUG, LOGFILE_DEBUG,
"%lu [poll_waitevents] " "%lu [poll_waitevents] "

44
server/core/service.c
View File

@ -354,7 +354,6 @@ GWPROTOCOL *funcs;
goto retblock; goto retblock;
} }
memcpy(&(port->listener->func), funcs, sizeof(GWPROTOCOL)); memcpy(&(port->listener->func), funcs, sizeof(GWPROTOCOL));
port->listener->session = NULL;
if (port->address) if (port->address)
sprintf(config_bind, "%s:%d", port->address, port->port); sprintf(config_bind, "%s:%d", port->address, port->port);
@ -363,21 +362,38 @@ GWPROTOCOL *funcs;
if (port->listener->func.listen(port->listener, config_bind)) if (port->listener->func.listen(port->listener, config_bind))
{ {
listeners += 1; port->listener->session = session_alloc(service, port->listener);
/* If lazy session creation fails, how does listeners get decremented? */
} if (port->listener->session != NULL)
else {
{ port->listener->session->state = SESSION_STATE_LISTENER;
LOGIF(LE, (skygw_log_write_flush( listeners += 1;
LOGFILE_ERROR, }
else
{
LOGIF(LE, (skygw_log_write_flush(
LOGFILE_ERROR,
"Error : Failed to create session to service %s.",
service->name)));
users_free(service->users);
dcb_close(port->listener);
port->listener = NULL;
goto retblock;
}
}
else
{
LOGIF(LE, (skygw_log_write_flush(
LOGFILE_ERROR,
"Error : Unable to start to listen port %d for %s %s.", "Error : Unable to start to listen port %d for %s %s.",
port->port, port->port,
port->protocol, port->protocol,
service->name))); service->name)));
users_free(service->users); users_free(service->users);
dcb_close(port->listener); dcb_close(port->listener);
port->listener = NULL; port->listener = NULL;
} }
retblock: retblock:
return listeners; return listeners;

308
server/core/session.c
View File

@ -58,6 +58,7 @@ static SESSION *allSessions = NULL;
static int session_setup_filters(SESSION *session); static int session_setup_filters(SESSION *session);
static void session_simple_free(SESSION *session, DCB *dcb);
/** /**
* Allocate a new session for a new client of the specified service. * Allocate a new session for a new client of the specified service.
@ -76,8 +77,7 @@ session_alloc(SERVICE *service, DCB *client_dcb)
SESSION *session; SESSION *session;
session = (SESSION *)calloc(1, sizeof(SESSION)); session = (SESSION *)calloc(1, sizeof(SESSION));
ss_info_dassert(session != NULL, ss_info_dassert(session != NULL, "Allocating memory for session failed.");
"Allocating memory for session failed.");
if (session == NULL) if (session == NULL)
{ {
@ -87,43 +87,29 @@ session_alloc(SERVICE *service, DCB *client_dcb)
"session object due error %d, %s.", "session object due error %d, %s.",
errno, errno,
strerror(errno)))); strerror(errno))));
if (client_dcb->data && !DCB_IS_CLONE(client_dcb)) session_simple_free(session, client_dcb);
{ return NULL;
void *clientdata = client_dcb->data;
client_dcb->data = NULL;
free(clientdata);
}
goto return_session;
} }
#if defined(SS_DEBUG) #if defined(SS_DEBUG)
session->ses_chk_top = CHK_NUM_SESSION; session->ses_chk_top = CHK_NUM_SESSION;
session->ses_chk_tail = CHK_NUM_SESSION; session->ses_chk_tail = CHK_NUM_SESSION;
#endif #endif
if (DCB_IS_CLONE(client_dcb)) session->ses_is_child = (bool) DCB_IS_CLONE(client_dcb);
{
session->ses_is_child = true;
}
spinlock_init(&session->ses_lock); spinlock_init(&session->ses_lock);
/*<
* Prevent backend threads from accessing before session is completely
* initialized.
*/
spinlock_acquire(&session->ses_lock);
session->service = service; session->service = service;
session->client = client_dcb; session->client = client_dcb;
session->n_filters = 0; session->n_filters = 0;
memset(&session->stats, 0, sizeof(SESSION_STATS)); memset(&session->stats, 0, sizeof(SESSION_STATS));
session->stats.connect = time(0); session->stats.connect = time(0);
session->state = SESSION_STATE_ALLOC; session->state = SESSION_STATE_ALLOC;
/*< /*<
* Associate the session to the client DCB and set the reference count on * Associate the session to the client DCB and set the reference count on
* the session to indicate that there is a single reference to the * the session to indicate that there is a single reference to the
* session. There is no need to protect this or use atomic add as the * session. There is no need to protect this or use atomic add as the
* session has not been made available to the other threads at this * session has not been made available to the other threads at this
* point. * point.
*/ */
session->data = client_dcb->data; session->data = client_dcb->data;
client_dcb->session = session;
session->refcount = 1; session->refcount = 1;
/*< /*<
* This indicates that session is ready to be shared with backend * This indicates that session is ready to be shared with backend
@ -131,139 +117,99 @@ session_alloc(SERVICE *service, DCB *client_dcb)
*/ */
session->state = SESSION_STATE_READY; session->state = SESSION_STATE_READY;
/*< Release session lock */ /*
spinlock_release(&session->ses_lock); * Only create a router session if we are not the listening
* DCB or an internal DCB. Creating a router session may create a connection to a
/* * backend server, depending upon the router module implementation
* Only create a router session if we are not the listening * and should be avoided for the listener session
* DCB or an internal DCB. Creating a router session may create a connection to a *
* backend server, depending upon the router module implementation * Router session creation may create other DCBs that link to the
* and should be avoided for the listener session * session, therefore it is important that the session lock is
*
* Router session creation may create other DCBs that link to the
* session, therefore it is important that the session lock is
* relinquished before the router call. * relinquished before the router call.
*/ */
if (client_dcb->state != DCB_STATE_LISTENING && if (client_dcb->state != DCB_STATE_LISTENING &&
client_dcb->dcb_role != DCB_ROLE_INTERNAL) client_dcb->dcb_role != DCB_ROLE_INTERNAL)
{ {
session->router_session = session->router_session = service->router->newSession(service->router_instance, session);
service->router->newSession(service->router_instance, session);
if (session->router_session == NULL) if (session->router_session == NULL)
{ {
/** session_simple_free(session, client_dcb);
* Inform other threads that session is closing.
*/
session->state = SESSION_STATE_STOPPING;
/*<
* Decrease refcount, set dcb's session pointer NULL
* and set session pointer to NULL.
*/
session->client = NULL;
session_free(session);
client_dcb->session = NULL;
session = NULL;
LOGIF(LE, (skygw_log_write_flush( LOGIF(LE, (skygw_log_write_flush(
LOGFILE_ERROR, LOGFILE_ERROR,
"Error : Failed to create %s session.", "%lu [%s] Error : Failed to create %s session because router"
"could not establish a new router session, see earlier error.",
pthread_self(),
__func__,
service->name))); service->name)));
goto return_session; return NULL;
} }
/* /*
* Pending filter chain being setup set the head of the chain to * Pending filter chain being setup set the head of the chain to
* be the router. As filters are inserted the current head will * be the router. As filters are inserted the current head will
* be pushed to the filter and the head updated. * be pushed to the filter and the head updated.
* *
* NB This dictates that filters are created starting at the end * NB This dictates that filters are created starting at the end
* of the chain nearest the router working back to the client * of the chain nearest the router working back to the client
* protocol end of the chain. * protocol end of the chain.
*/ */
session->head.instance = service->router_instance; session->head.instance = service->router_instance;
session->head.session = session->router_session; session->head.session = session->router_session;
session->head.routeQuery = (void *)(service->router->routeQuery); session->head.routeQuery = (void *)(service->router->routeQuery);
session->tail.instance = session; session->tail.instance = session;
session->tail.session = session; session->tail.session = session;
session->tail.clientReply = session_reply; session->tail.clientReply = session_reply;
if (service->n_filters > 0) if (service->n_filters > 0)
{
if (!session_setup_filters(session))
{
/**
* Inform other threads that session is closing.
*/
session->state = SESSION_STATE_STOPPING;
/*<
* Decrease refcount, set dcb's session pointer NULL
* and set session pointer to NULL.
*/
session->client = NULL;
session_free(session);
client_dcb->session = NULL;
session = NULL;
LOGIF(LE, (skygw_log_write(
LOGFILE_ERROR,
"Error : Setting up filters failed. "
"Terminating session %s.",
service->name)));
goto return_session;
}
}
}
spinlock_acquire(&session->ses_lock);
if (session->state != SESSION_STATE_READY)
{ {
spinlock_release(&session->ses_lock); if (!session_setup_filters(session))
session->client = NULL; {
session_free(session); session_simple_free(session, client_dcb);
client_dcb->session = NULL; LOGIF(LE, (skygw_log_write(
session = NULL; LOGFILE_ERROR,
LOGIF(LE, (skygw_log_write_flush( "Error : Setting up filters failed. "
LOGFILE_ERROR, "Terminating session %s.",
"Error : Failed to create %s session.", service->name)));
service->name))); return NULL;
spinlock_release(&session_spin); }
} }
else }
{
session->state = SESSION_STATE_ROUTER_READY; session->state = SESSION_STATE_ROUTER_READY;
spinlock_release(&session->ses_lock); spinlock_acquire(&session_spin);
spinlock_acquire(&session_spin); /** Assign a session id and increase, insert session into list */
/** Assign a session id and increase */ session->ses_id = ++session_id;
session->ses_id = ++session_id; session->next = allSessions;
session->next = allSessions; allSessions = session;
allSessions = session; spinlock_release(&session_spin);
spinlock_release(&session_spin);
if (session->client->user == NULL) if (session->client->user == NULL)
{ {
LOGIF(LT, (skygw_log_write( LOGIF(LT, (skygw_log_write(
LOGFILE_TRACE, LOGFILE_TRACE,
"Started session [%lu] for %s service ", "Started session [%lu] for %s service ",
session->ses_id, session->ses_id,
service->name))); service->name)));
} }
else else
{ {
LOGIF(LT, (skygw_log_write( LOGIF(LT, (skygw_log_write(
LOGFILE_TRACE, LOGFILE_TRACE,
"Started %s client session [%lu] for '%s' from %s", "Started %s client session [%lu] for '%s' from %s",
service->name, service->name,
session->ses_id, session->ses_id,
session->client->user, session->client->user,
session->client->remote))); session->client->remote)));
} }
atomic_add(&service->stats.n_sessions, 1); atomic_add(&service->stats.n_sessions, 1);
atomic_add(&service->stats.n_current, 1); atomic_add(&service->stats.n_current, 1);
CHK_SESSION(session); CHK_SESSION(session);
}
return_session: client_dcb->session = session;
return session; return session;
} }
/** /**
@ -360,30 +306,57 @@ int session_unlink_dcb(
return nlink; return nlink;
} }
/**
* Deallocate the specified session, minimal actions during session_alloc
*
* @param session The session to deallocate
*/
static void
session_simple_free(SESSION *session, DCB *dcb)
{
/* Does this possibly need a lock? */
if (dcb->data && !DCB_IS_CLONE(dcb))
{
void * clientdata = dcb->data;
dcb->data = NULL;
free(clientdata);
}
if (session)
{
if (session && session->router_session)
{
session->service->router->freeSession(
session->service->router_instance,
session->router_session);
}
session->state = SESSION_STATE_STOPPING;
}
free(session);
}
/** /**
* Deallocate the specified session * Deallocate the specified session
* *
* @param session The session to deallocate * @param session The session to deallocate
*/ */
bool session_free( bool
SESSION *session) session_free(SESSION *session)
{ {
bool succp = false;
SESSION *ptr;
int nlink;
int i;
CHK_SESSION(session); CHK_SESSION(session);
/*<
/*
* Remove one reference. If there are no references left, * Remove one reference. If there are no references left,
* free session. * free session.
*/ */
nlink = session_unlink_dcb(session, NULL); if (atomic_add(&session->refcount, -1) - 1)
{
if (nlink != 0) { /* Must be one or more references left */
ss_dassert(nlink > 0); ss_dassert(nlink > 0);
goto return_succp; return false;
} }
session->state = SESSION_STATE_TO_BE_FREED;
/* First of all remove from the linked list */ /* First of all remove from the linked list */
spinlock_acquire(&session_spin); spinlock_acquire(&session_spin);
@ -393,13 +366,16 @@ bool session_free(
} }
else else
{ {
ptr = allSessions; SESSION *chksession;
while (ptr && ptr->next != session) chksession = allSessions;
while (chksession && chksession->next != session)
{ {
ptr = ptr->next; chksession = chksession->next;
} }
if (ptr) if (chksession)
ptr->next = session->next; {
chksession->next = session->next;
}
} }
spinlock_release(&session_spin); spinlock_release(&session_spin);
atomic_add(&session->service->stats.n_current, -1); atomic_add(&session->service->stats.n_current, -1);
@ -416,6 +392,7 @@ bool session_free(
} }
if (session->n_filters) if (session->n_filters)
{ {
int i;
for (i = 0; i < session->n_filters; i++) for (i = 0; i < session->n_filters; i++)
{ {
if (session->filters[i].filter) if (session->filters[i].filter)
@ -453,10 +430,7 @@ bool session_free(
} }
free(session); free(session);
} }
succp = true; return true;
return_succp :
return succp;
} }
/** /**

6
server/modules/protocol/httpd.c
View File

@ -351,7 +351,8 @@ int n_connect = 0;
memcpy(&client->func, &MyObject, sizeof(GWPROTOCOL)); memcpy(&client->func, &MyObject, sizeof(GWPROTOCOL));
/* we don't need the session */ /* we don't need the session */
client->session = NULL; /* But not clear that we have one! */
/* client->session = NULL; */
/* create the session data for HTTPD */ /* create the session data for HTTPD */
client_data = (HTTPD_session *)calloc(1, sizeof(HTTPD_session)); client_data = (HTTPD_session *)calloc(1, sizeof(HTTPD_session));
@ -360,9 +361,10 @@ int n_connect = 0;
client->session = client->session =
session_alloc(dcb->session->service, client); session_alloc(dcb->session->service, client);
if (poll_add_dcb(client) == -1) if (NULL == client->session || poll_add_dcb(client) == -1)
{ {
close(so); close(so);
dcb_close(client);
return n_connect; return n_connect;
} }
n_connect++; n_connect++;

2
server/modules/protocol/maxscaled.c
View File

@ -291,7 +291,7 @@ int n_connect = 0;
client_dcb->session = client_dcb->session =
session_alloc(dcb->session->service, client_dcb); session_alloc(dcb->session->service, client_dcb);
if (poll_add_dcb(client_dcb)) if (NULL == client_dcb->session || poll_add_dcb(client_dcb))
{ {
dcb_close(dcb); dcb_close(dcb);
return n_connect; return n_connect;

5
server/modules/protocol/telnetd.c
View File

@ -311,6 +311,11 @@ int n_connect = 0;
memcpy(&client_dcb->func, &MyObject, sizeof(GWPROTOCOL)); memcpy(&client_dcb->func, &MyObject, sizeof(GWPROTOCOL));
client_dcb->session = client_dcb->session =
session_alloc(dcb->session->service, client_dcb); session_alloc(dcb->session->service, client_dcb);
if (NULL == client_dcb->session)
{
dcb_close(client_dcb);
return n_connect;
}
telnetd_pr = (TELNETD *)malloc(sizeof(TELNETD)); telnetd_pr = (TELNETD *)malloc(sizeof(TELNETD));
client_dcb->protocol = (void *)telnetd_pr; client_dcb->protocol = (void *)telnetd_pr;

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

@ -1684,7 +1684,7 @@ static skygw_query_type_t is_read_tmp_table(
{ {
skygw_log_write(LE,"[%s] Error: Master server reference is NULL.", skygw_log_write(LE,"[%s] Error: Master server reference is NULL.",
__FUNCTION__); __FUNCTION__);
return; return type;
} }
rses_prop_tmp = router_cli_ses->rses_properties[RSES_PROP_TYPE_TMPTABLES]; rses_prop_tmp = router_cli_ses->rses_properties[RSES_PROP_TYPE_TMPTABLES];