From 00fded016b573bba5dc1b1ab072d4c319ec5f1bd Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Fri, 31 Oct 2014 15:25:59 +0200 Subject: [PATCH] Fixes to Coverity tasks : 73267, 72686, 72672 Cleaned up warnings, and added checks to malloc return values and error log writes in case of failures. --- log_manager/log_manager.cc | 1 - log_manager/log_manager.h | 5 +- server/core/buffer.c | 49 +++++++++++++++++-- server/core/dcb.c | 1 - server/core/modutil.c | 12 +++-- server/modules/protocol/mysql_backend.c | 23 ++++++++- server/modules/protocol/mysql_common.c | 29 ++++++++--- server/modules/routing/readconnroute.c | 24 ++++----- .../routing/readwritesplit/readwritesplit.c | 11 ++++- 9 files changed, 123 insertions(+), 32 deletions(-) diff --git a/log_manager/log_manager.cc b/log_manager/log_manager.cc index 9caafc676..792cc934a 100644 --- a/log_manager/log_manager.cc +++ b/log_manager/log_manager.cc @@ -273,7 +273,6 @@ static char* add_slash(char* str); static bool check_file_and_path( char* filename, - bool* nameconflict, bool* writable); static bool file_is_symlink(char* filename); diff --git a/log_manager/log_manager.h b/log_manager/log_manager.h index c6d207702..776b6cbb7 100644 --- a/log_manager/log_manager.h +++ b/log_manager/log_manager.h @@ -15,7 +15,8 @@ * * Copyright MariaDB Corporation Ab 2013-2014 */ - +#if !defined(LOG_MANAGER_H) +# define LOG_MANAGER_H typedef struct filewriter_st filewriter_t; typedef struct logfile_st logfile_t; @@ -90,3 +91,5 @@ const char* get_msg_suffix_default(void); const char* get_err_prefix_default(void); const char* get_err_suffix_default(void); const char* get_logpath_default(void); + +#endif /** LOG_MANAGER_H */ diff --git a/server/core/buffer.c b/server/core/buffer.c index 81d7bd629..45ddfd1cc 100644 --- a/server/core/buffer.c +++ b/server/core/buffer.c @@ -44,6 +44,10 @@ #include #include #include +#include +#include + +extern int lm_enabled_logfiles_bitmask; static buffer_object_t* gwbuf_remove_buffer_object( GWBUF* buf, @@ -70,22 +74,23 @@ SHARED_BUF *sbuf; /* Allocate the buffer header */ if ((rval = (GWBUF *)malloc(sizeof(GWBUF))) == NULL) { - return NULL; + goto retblock;; } /* Allocate the shared data buffer */ if ((sbuf = (SHARED_BUF *)malloc(sizeof(SHARED_BUF))) == NULL) { free(rval); - return NULL; + goto retblock; } /* Allocate the space for the actual data */ if ((sbuf->data = (unsigned char *)malloc(size)) == NULL) { + ss_dassert(sbuf->data != NULL); free(rval); free(sbuf); - return NULL; + goto retblock; } spinlock_init(&rval->gwbuf_lock); rval->start = sbuf->data; @@ -100,6 +105,15 @@ SHARED_BUF *sbuf; rval->gwbuf_info = GWBUF_INFO_NONE; rval->gwbuf_bufobj = NULL; CHK_GWBUF(rval); +retblock: + if (rval == NULL || sbuf == NULL || sbuf->data == NULL) + { + ss_dassert(rval != NULL && sbuf != NULL && sbuf->data != NULL); + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Memory allocation failed due to %s.", + strerror(errno)))); + } return rval; } @@ -163,6 +177,11 @@ GWBUF *rval; if ((rval = (GWBUF *)malloc(sizeof(GWBUF))) == NULL) { + ss_dassert(rval != NULL); + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Memory allocation failed due to %s.", + strerror(errno)))); return NULL; } @@ -194,6 +213,11 @@ GWBUF *gwbuf_clone_portion( if ((clonebuf = (GWBUF *)malloc(sizeof(GWBUF))) == NULL) { + ss_dassert(clonebuf != NULL); + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Memory allocation failed due to %s.", + strerror(errno)))); return NULL; } atomic_add(&buf->sbuf->refcount, 1); @@ -438,6 +462,16 @@ void gwbuf_add_buffer_object( CHK_GWBUF(buf); newb = (buffer_object_t *)malloc(sizeof(buffer_object_t)); + ss_dassert(newb != NULL); + + if (newb == NULL) + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Memory allocation failed due to %s.", + strerror(errno)))); + return; + } newb->bo_id = id; newb->bo_data = data; newb->bo_donefun_fp = donefun_fp; @@ -518,8 +552,15 @@ gwbuf_add_property(GWBUF *buf, char *name, char *value) BUF_PROPERTY *prop; if ((prop = malloc(sizeof(BUF_PROPERTY))) == NULL) + { + ss_dassert(prop != NULL); + + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Memory allocation failed due to %s.", + strerror(errno)))); return 0; - + } prop->name = strdup(name); prop->value = strdup(value); spinlock_acquire(&buf->gwbuf_lock); diff --git a/server/core/dcb.c b/server/core/dcb.c index 36e088dfc..acd553360 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -1891,7 +1891,6 @@ DCB_CALLBACK *cb, *nextcb; int dcb_isvalid(DCB *dcb) { -DCB *ptr; int rval = 0; if (dcb) diff --git a/server/core/modutil.c b/server/core/modutil.c index e5b557cc3..cd596fb2b 100644 --- a/server/core/modutil.c +++ b/server/core/modutil.c @@ -245,7 +245,7 @@ GWBUF *modutil_create_mysql_err_msg( int affected_rows, int merrno, const char *statemsg, - const char * msg) + const char *msg) { uint8_t *outbuf = NULL; uint8_t mysql_payload_size = 0; @@ -259,6 +259,10 @@ GWBUF *modutil_create_mysql_err_msg( const char *mysql_state = NULL; GWBUF *errbuf = NULL; + if (statemsg == NULL || msg == NULL) + { + return NULL; + } mysql_errno = (unsigned int)merrno; mysql_error_msg = msg; mysql_state = statemsg; @@ -270,9 +274,6 @@ GWBUF *modutil_create_mysql_err_msg( mysql_statemsg[0]='#'; memcpy(mysql_statemsg+1, mysql_state, 5); - if (msg != NULL) { - mysql_error_msg = msg; - } mysql_payload_size = sizeof(field_count) + sizeof(mysql_err) + sizeof(mysql_statemsg) + @@ -283,8 +284,9 @@ GWBUF *modutil_create_mysql_err_msg( ss_dassert(errbuf != NULL); if (errbuf == NULL) + { return NULL; - + } outbuf = GWBUF_DATA(errbuf); /** write packet header and packet number */ diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 35e75a279..c006c3788 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -1300,14 +1300,33 @@ static int gw_change_user( backend->session->client->remote, password_set, ""); + if (message == NULL) + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Creating error message failed."))); + rv = 0; + goto retblock; + } /** TODO: Add custom message indicating that retry would probably help */ buf = modutil_create_mysql_err_msg(1, 0, 1045, "28000", message); + free(message); + + if (buf == NULL) + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Creating buffer for error message failed."))); + rv = 0; + goto retblock; + } /** Set flags that help router to identify session commans reply */ gwbuf_set_type(buf, GWBUF_TYPE_MYSQL); gwbuf_set_type(buf, GWBUF_TYPE_SESCMD_RESPONSE); gwbuf_set_type(buf, GWBUF_TYPE_RESPONSE_END); /** Create an incoming event for backend DCB */ - poll_add_epollin_event_to_dcb(backend, buf); + poll_add_epollin_event_to_dcb(backend, gwbuf_clone(buf)); + gwbuf_free(buf); rv = 0; } else { rv = gw_send_change_user_to_backend(database, username, client_sha1, backend_protocol); @@ -1318,6 +1337,8 @@ static int gw_change_user( strcpy(current_session->db, database); memcpy(current_session->client_sha1, client_sha1, sizeof(current_session->client_sha1)); } + +retblock: gwbuf_free(queue); return rv; diff --git a/server/modules/protocol/mysql_common.c b/server/modules/protocol/mysql_common.c index 00bdd0e3f..b46e1e1ef 100644 --- a/server/modules/protocol/mysql_common.c +++ b/server/modules/protocol/mysql_common.c @@ -2106,19 +2106,34 @@ char *create_auth_fail_str( else db_len = 0; - if (db_len>0) + if (db_len > 0) + { ferrstr = "Access denied for user '%s'@'%s' (using password: %s) to database '%s'"; + } else + { ferrstr = "Access denied for user '%s'@'%s' (using password: %s)"; - + } errstr = (char *)malloc(strlen(username)+strlen(ferrstr)+strlen(hostaddr)+strlen("YES")-6 + db_len + ((db_len > 0) ? (strlen(" to database ") +2) : 0) + 1); - if (errstr != NULL) { - if (db_len>0) - sprintf(errstr, ferrstr, username, hostaddr, (*sha1 == '\0' ? "NO" : "YES"), db); - else - sprintf(errstr, ferrstr, username, hostaddr, (*sha1 == '\0' ? "NO" : "YES")); + if (errstr == NULL) + { + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Memory allocation failed due to %s.", + strerror(errno)))); + goto retblock; } + if (db_len > 0) + { + sprintf(errstr, ferrstr, username, hostaddr, (*sha1 == '\0' ? "NO" : "YES"), db); + } + else + { + sprintf(errstr, ferrstr, username, hostaddr, (*sha1 == '\0' ? "NO" : "YES")); + } + +retblock: return errstr; } diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index 9ccbb536a..b8aad7ae9 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -654,7 +654,7 @@ DCB* backend_dcb; * @param instance The router instance * @param router_session The router session returned from the newSession call * @param queue The queue of data buffers to route - * @return The number of bytes sent + * @return if succeed 1, otherwise 0 */ static int routeQuery(ROUTER *instance, void *router_session, GWBUF *queue) @@ -697,20 +697,22 @@ routeQuery(ROUTER *instance, void *router_session, GWBUF *queue) "Error : Failed to route MySQL command %d to backend " "server.", mysql_command))); + rc = 0; goto return_rc; } switch(mysql_command) { - case MYSQL_COM_CHANGE_USER: - rc = backend_dcb->func.auth( - backend_dcb, - NULL, - backend_dcb->session, - queue); - break; - default: - rc = backend_dcb->func.write(backend_dcb, queue); - break; + case MYSQL_COM_CHANGE_USER: + rc = backend_dcb->func.auth( + backend_dcb, + NULL, + backend_dcb->session, + queue); + break; + + default: + rc = backend_dcb->func.write(backend_dcb, queue); + break; } CHK_PROTOCOL(((MySQLProtocol*)backend_dcb->protocol)); diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 82476d418..f76eab3d3 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -3829,7 +3829,6 @@ static bool route_session_write( { succp = false; } - } } rses_end_locked_router_action(router_cli_ses); @@ -3842,6 +3841,12 @@ static bool route_session_write( succp = false; goto return_succp; } + + if (router_cli_ses->rses_nbackends <= 0) + { + succp = false; + goto return_succp; + } /** * Additional reference is created to querybuf to * prevent it from being released before properties @@ -3909,6 +3914,10 @@ static bool route_session_write( } } } + else + { + succp = false; + } } /** Unlock router session */ rses_end_locked_router_action(router_cli_ses);