From 4b98c472a8f9ad860e74f7d820e2bd0ef066c307 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Thu, 6 Nov 2014 16:12:01 +0200 Subject: [PATCH 1/7] Fixed a comment --- server/modules/protocol/mysql_backend.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 868647cfb..70f8a3a44 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -453,12 +453,13 @@ static int gw_read_backend_event(DCB *dcb) { 0, "Read from backend failed"); - router->handleError(router_instance, - session->router_session, - errbuf, - dcb, - ERRACT_NEW_CONNECTION, - &succp); + router->handleError( + router_instance, + session->router_session, + errbuf, + dcb, + ERRACT_NEW_CONNECTION, + &succp); gwbuf_free(errbuf); if (!succp) @@ -1170,7 +1171,7 @@ static int backend_write_delayqueue(DCB *dcb) 0, "Failed to write buffered data to back-end server. " "Buffer was empty or back-end was disconnected during " - "operation. Session will be closed."); + "operation. Attempting to find a new backend."); router->handleError(router_instance, rsession, From 8cfea996e7b38a655e7e0d214f9894dede02ea4d Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Thu, 6 Nov 2014 20:04:18 +0200 Subject: [PATCH 2/7] Fixed an unassigned pointer causing memory corruption. --- .../modules/routing/readwritesplit/readwritesplit.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index e7f003d04..569f9d148 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1406,7 +1406,7 @@ void check_drop_tmp_table( { int tsize = 0, klen = 0,i; - char** tbl; + char** tbl = NULL; char *hkey,*dbname; MYSQL_session* data; @@ -1447,7 +1447,9 @@ void check_drop_tmp_table( free(tbl[i]); free(hkey); } - free(tbl); + if(tbl != NULL){ + free(tbl); + } } } @@ -1468,7 +1470,7 @@ skygw_query_type_t is_read_tmp_table( bool target_tmp_table = false; int tsize = 0, klen = 0,i; - char** tbl; + char** tbl = NULL; char *hkey,*dbname; MYSQL_session* data; @@ -1529,7 +1531,10 @@ skygw_query_type_t is_read_tmp_table( { free(tbl[i]); } - free(tbl); + + if(tbl != NULL){ + free(tbl); + } return qtype; } From 1ed3c9cc625eb8e2566e37064ec0714651d7eeab Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Thu, 6 Nov 2014 22:24:12 +0200 Subject: [PATCH 3/7] Fix to Coverity issues 72731 and 72708 In routeQuery: check if master has failed and in that case abort routing with an error sent back to the client. handle_error_new_connection also tests for master failure and returns with error if that is the case. --- .../routing/readwritesplit/readwritesplit.c | 114 +++++++++++++----- 1 file changed, 81 insertions(+), 33 deletions(-) diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index 569f9d148..4547fd32c 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1719,27 +1719,41 @@ static int routeQuery( LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, - "Error: Failed to route %s:%s:\"%s\" to " - "backend server. %s.", + "Error: Can't route %s:%s:\"%s\" to " + "backend server. Router is closed.", STRPACKETTYPE(packet_type), STRQTYPE(qtype), - (query_str == NULL ? "(empty)" : query_str), - (rses_is_closed ? "Router was closed" : - "Router has no backend servers where to " - "route to")))); + (query_str == NULL ? "(empty)" : query_str)))); + free(query_str); } + ret = 0; goto retblock; } + + /** Read stored master DCB pointer */ + if ((master_dcb = router_cli_ses->rses_master_ref->bref_dcb) == NULL) + { + char* query_str = modutil_get_query(querybuf); + CHK_DCB(master_dcb); + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error: Can't route %s:%s:\"%s\" to " + "backend server. Session doesn't have a Master " + "node", + STRPACKETTYPE(packet_type), + STRQTYPE(qtype), + (query_str == NULL ? "(empty)" : query_str)))); + free(query_str); + ret = 0; + goto retblock; + } + /** If buffer is not contiguous, make it such */ if (querybuf->next != NULL) { querybuf = gwbuf_make_contiguous(querybuf); } - - /** Read stored master DCB pointer */ - master_dcb = router_cli_ses->rses_master_ref->bref_dcb; - CHK_DCB(master_dcb); - + switch(packet_type) { case MYSQL_COM_QUIT: /*< 1 QUIT will close all sessions */ case MYSQL_COM_INIT_DB: /*< 2 DDL must go to the master */ @@ -2035,13 +2049,13 @@ static int routeQuery( NULL, MAX_RLAG_UNDEFINED); - if (succp && (master_dcb == NULL || master_dcb == curr_master_dcb)) + if (succp && master_dcb == curr_master_dcb) { atomic_add(&inst->stats.n_master, 1); target_dcb = master_dcb; } else - { + { if (succp && master_dcb != curr_master_dcb) { LOGIF(LT, (skygw_log_write(LOGFILE_TRACE, @@ -4082,6 +4096,22 @@ static void handleError ( return; } + if (rses->rses_master_ref->bref_dcb == backend_dcb) + { + /** Master failed, can't recover */ + LOGIF(LE, (skygw_log_write_flush( + LOGFILE_ERROR, + "Error : Master node have failed. " + "Session will be closed."))); + + *succp = false; + rses_end_locked_router_action(rses); + return; + } + /** + * This is called in hope of getting replacement for + * failed slave(s). + */ *succp = handle_error_new_connection(inst, rses, backend_dcb, @@ -4124,7 +4154,18 @@ static void handle_error_reply_client( } /** - * This must be called with router lock + * Check if there is backend reference pointing at failed DCB, and reset its + * flags. Then clear DCB's callback and finally : try to find replacement(s) + * for failed slave(s). + * + * This must be called with router lock. + * + * @param inst router instance + * @param rses router client session + * @param dcb failed DCB + * @param errmsg error message which is sent to client if it is waiting + * + * @return true if there are enough backend connections to continue, false if not */ static bool handle_error_new_connection( ROUTER_INSTANCE* inst, @@ -4144,25 +4185,14 @@ static bool handle_error_new_connection( ses = backend_dcb->session; CHK_SESSION(ses); - bref = get_bref_from_dcb(rses, backend_dcb); - - /** failed DCB has already been replaced */ - if (bref == NULL) - { - succp = true; - goto return_succp; - } - /** - * Error handler is already called for this DCB because - * it's not polling anymore. It can be assumed that - * it succeed because rses isn't closed. + /** + * If bref == NULL it has been replaced already with another one. */ - if (backend_dcb->state != DCB_STATE_POLLING) + if ((bref = get_bref_from_dcb(rses, backend_dcb)) == NULL) { succp = true; goto return_succp; } - CHK_BACKEND_REF(bref); if (BREF_IS_WAITING_RESULT(bref)) @@ -4174,6 +4204,17 @@ static bool handle_error_new_connection( } bref_clear_state(bref, BREF_IN_USE); bref_set_state(bref, BREF_CLOSED); + + /** + * Error handler is already called for this DCB because + * it's not polling anymore. It can be assumed that + * it succeed because rses isn't closed. + */ + if (backend_dcb->state != DCB_STATE_POLLING) + { + succp = true; + goto return_succp; + } /** * Remove callback because this DCB won't be used * unless it is reconnected later, and then the callback @@ -4298,8 +4339,8 @@ static bool have_enough_servers( } else { - double pct = (*p_rses)->rses_config.rw_max_slave_conn_percent/100; - double nservers = (double)router_nsrv*pct; + int pct = (*p_rses)->rses_config.rw_max_slave_conn_percent/100; + int nservers = router_nsrv*pct; if ((*p_rses)->rses_config.rw_max_slave_conn_count < min_nsrv) { @@ -4318,11 +4359,11 @@ static bool have_enough_servers( LOGFILE_ERROR, "Error : Unable to start %s service. There are " "too few backend servers configured in " - "MaxScale.cnf. Found %d%% when at least %.0f%% " + "MaxScale.cnf. Found %d%% when at least %d%% " "would be required.", router->service->name, (*p_rses)->rses_config.rw_max_slave_conn_percent, - min_nsrv/(((double)router_nsrv)/100)))); + min_nsrv/(router_nsrv/100)))); } } free(*p_rses); @@ -4385,7 +4426,14 @@ static int rses_get_max_replication_lag( return conf_max_rlag; } - +/** + * Finds out if there is a backend reference pointing at the DCB given as + * parameter. + * @param rses router client session + * @param dcb DCB + * + * @return backend reference pointer if succeed or NULL + */ static backend_ref_t* get_bref_from_dcb( ROUTER_CLIENT_SES* rses, DCB* dcb) From 4f39828fa1a54aeea6b337c3eb861cb4a1488a7e Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Fri, 7 Nov 2014 10:07:22 +0200 Subject: [PATCH 4/7] Fixes to Coveriry issues 72757 & 73266 --- log_manager/log_manager.cc | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/log_manager/log_manager.cc b/log_manager/log_manager.cc index 792cc934a..75d6614d9 100644 --- a/log_manager/log_manager.cc +++ b/log_manager/log_manager.cc @@ -423,9 +423,17 @@ static bool logmanager_init_nomutex( return_succp: if (err != 0) { - skygw_message_done(lm->lm_clientmes); - skygw_message_done(lm->lm_logmes); - + if (lm != NULL) + { + if (lm->lm_clientmes != NULL) + { + skygw_message_done(lm->lm_clientmes); + } + if (lm->lm_logmes != NULL) + { + skygw_message_done(lm->lm_logmes); + } + } /** This releases memory of all created objects */ logmanager_done_nomutex(); fprintf(stderr, "*\n* Error : Initializing log manager failed.\n*\n"); @@ -1895,15 +1903,18 @@ static char* form_full_file_name( fprintf(stderr, "Error : Too long file name= %d.\n", (int)fnlen); goto return_filename; } - filename = (char*)calloc(1, fnlen); - snprintf(seqnostr, s+1, "%d", seqno); - + + if (seqnostr != NULL) + { + snprintf(seqnostr, s+1, "%d", seqno); + } + for (i=0, p=parts; p->sp_string != NULL; i++, p=p->sp_next) { - if (i == seqnoidx) + if (seqnostr != NULL && i == seqnoidx) { - strcat(filename, seqnostr); + strcat(filename, seqnostr); /*< add sequence number */ } strcat(filename, p->sp_string); From e1af60ac712e672ec4dcabc7e87be1a330e6d473 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 7 Nov 2014 10:57:11 +0200 Subject: [PATCH 5/7] Fixes to Coverity errors: 72747 75940 75941 73408 75425 --- server/core/maxpasswd.c | 18 +++++++++++++++--- server/core/test/testbuffer.c | 4 +++- server/modules/filter/regexfilter.c | 2 +- server/modules/filter/tee.c | 1 + server/modules/filter/test/harness_common.c | 2 +- 5 files changed, 21 insertions(+), 6 deletions(-) diff --git a/server/core/maxpasswd.c b/server/core/maxpasswd.c index 8d90a7631..0c728d869 100644 --- a/server/core/maxpasswd.c +++ b/server/core/maxpasswd.c @@ -39,7 +39,7 @@ int main(int argc, char **argv) { -char *enc; + char *enc, *pw; if (argc != 2) { @@ -47,9 +47,21 @@ char *enc; exit(1); } - if ((enc = encryptPassword(argv[1])) != NULL) + pw = calloc(81,sizeof(char)); + + if(pw == NULL){ + fprintf(stderr, "Error: cannot allocate enough memory."); + exit(1); + } + + strncpy(pw,argv[1],80); + + if ((enc = encryptPassword(pw)) != NULL){ printf("%s\n", enc); - else + }else{ fprintf(stderr, "Failed to encode the password\n"); + } + + free(pw); return 0; } diff --git a/server/core/test/testbuffer.c b/server/core/test/testbuffer.c index ee8507161..73d71dc27 100644 --- a/server/core/test/testbuffer.c +++ b/server/core/test/testbuffer.c @@ -61,7 +61,9 @@ int buflen; ss_info_dassert(0 == GWBUF_EMPTY(buffer), "Buffer should not be empty"); ss_info_dassert(GWBUF_IS_TYPE_UNDEFINED(buffer), "Buffer type should be undefined"); ss_dfprintf(stderr, "\t..done\nSet a hint for the buffer"); - hint = hint_create_parameter(NULL, strdup("name"), "value"); + char* name = strdup("name"); + hint = hint_create_parameter(NULL, name, "value"); + free(name); gwbuf_add_hint(buffer, hint); ss_info_dassert(hint == buffer->hint, "Buffer should point to first and only hint"); ss_dfprintf(stderr, "\t..done\nSet a property for the buffer"); diff --git a/server/modules/filter/regexfilter.c b/server/modules/filter/regexfilter.c index b76442079..53a775f83 100644 --- a/server/modules/filter/regexfilter.c +++ b/server/modules/filter/regexfilter.c @@ -302,7 +302,7 @@ routeQuery(FILTER *instance, void *session, GWBUF *queue) REGEX_INSTANCE *my_instance = (REGEX_INSTANCE *)instance; REGEX_SESSION *my_session = (REGEX_SESSION *)session; char *sql, *newsql; -int length; +int length = 0; if (modutil_is_SQL(queue)) { diff --git a/server/modules/filter/tee.c b/server/modules/filter/tee.c index 3b35d7b8e..94dc29ea0 100644 --- a/server/modules/filter/tee.c +++ b/server/modules/filter/tee.c @@ -415,6 +415,7 @@ GWBUF *clone = NULL; modutil_MySQL_Query(queue, &dummy, &length, &residual); clone = gwbuf_clone(queue); my_session->residual = residual; + free(ptr); } } diff --git a/server/modules/filter/test/harness_common.c b/server/modules/filter/test/harness_common.c index 29ed2cf53..a345a5e09 100644 --- a/server/modules/filter/test/harness_common.c +++ b/server/modules/filter/test/harness_common.c @@ -303,7 +303,7 @@ int load_query() int i, qcount = 0, qbuff_sz = 10, rval = 0; int offset = 0; unsigned int qlen = 0; - buffer = (char*)malloc(4092*sizeof(char)); + buffer = (char*)calloc(4092,sizeof(char)); if(buffer == NULL){ printf("Error: cannot allocate enough memory.\n"); skygw_log_write(LOGFILE_ERROR,"Error: cannot allocate enough memory.\n"); From 474f018ceed329a26a9f3296e334719be9acb781 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 7 Nov 2014 11:12:26 +0200 Subject: [PATCH 6/7] Fixes to minor Coverity errors: 75424 73422 72724 72702 72662 --- log_manager/test/testorder.c | 5 +++-- query_classifier/test/canonical_tests/canonizer.c | 3 +++ server/core/config.c | 4 ++-- server/core/gateway.c | 6 +++++- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/log_manager/test/testorder.c b/log_manager/test/testorder.c index 047e57966..f9bf14213 100644 --- a/log_manager/test/testorder.c +++ b/log_manager/test/testorder.c @@ -45,8 +45,9 @@ int main(int argc, char** argv) } block_size = atoi(argv[3]); - if(block_size < 1){ - fprintf(stderr,"Message size too small, must be at least 1 byte long."); + if(block_size < 1 || block_size > 1024){ + fprintf(stderr,"Message size too small or large, must be at least 1 byte long and must not exceed 1024 bytes."); + return 1; } diff --git a/query_classifier/test/canonical_tests/canonizer.c b/query_classifier/test/canonical_tests/canonizer.c index 223f91f2a..1de287015 100644 --- a/query_classifier/test/canonical_tests/canonizer.c +++ b/query_classifier/test/canonical_tests/canonizer.c @@ -57,6 +57,9 @@ int main(int argc, char** argv) { fgets(readbuff,4092,infile); psize = strlen(readbuff); + if(psize < 0 || > 4092){ + continue; + } qbuff = gwbuf_alloc(psize + 7); *(qbuff->sbuf->data + 0) = (unsigned char)psize; *(qbuff->sbuf->data + 1) = (unsigned char)(psize>>8); diff --git a/server/core/config.c b/server/core/config.c index 4ee39fcb2..1532909d0 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -1268,7 +1268,7 @@ SERVER *server; (PERCENT_TYPE|COUNT_TYPE)); } - if (!succp) + if (!succp && param != NULL) { LOGIF(LM, (skygw_log_write( LOGFILE_MESSAGE, @@ -1362,7 +1362,7 @@ SERVER *server; if (enable_root_user && service) serviceEnableRootUser(service, atoi(enable_root_user)); - if (allow_localhost_match_wildcard_host) + if (allow_localhost_match_wildcard_host && service) serviceEnableLocalhostMatchWildcardHost( service, atoi(allow_localhost_match_wildcard_host)); diff --git a/server/core/gateway.c b/server/core/gateway.c index d3f196f86..2c2c11e89 100644 --- a/server/core/gateway.c +++ b/server/core/gateway.c @@ -1479,7 +1479,11 @@ int main(int argc, char **argv) bool succp; sprintf(buf, "%s/log", home_dir); - mkdir(buf, 0777); + if(mkdir(buf, 0777) != 0){ + fprintf(stderr, + "Error: Cannot create log directory: %s\n",buf); + goto return_main; + } argv[0] = "MaxScale"; argv[1] = "-j"; argv[2] = buf; From a4caac55c8a9dfa5dd8153cccd6fc41c68c4c48e Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Fri, 7 Nov 2014 11:52:40 +0200 Subject: [PATCH 7/7] Fixes to Coverity errors: 72662 72702 72724 73397 73410 73414 73422 75424 75748 75789 75938 75939 Also includes a fix to a bug caused by a previous Coverity error change in canonizer.c --- query_classifier/query_classifier.cc | 43 ++++++++-------- .../test/canonical_tests/canonizer.c | 2 +- server/core/config.c | 2 +- server/modules/filter/test/harness_common.c | 15 +++--- .../routing/readwritesplit/readwritesplit.c | 50 +++++++++---------- 5 files changed, 57 insertions(+), 55 deletions(-) diff --git a/query_classifier/query_classifier.cc b/query_classifier/query_classifier.cc index 5773c3685..be25f239e 100644 --- a/query_classifier/query_classifier.cc +++ b/query_classifier/query_classifier.cc @@ -1088,30 +1088,31 @@ char** skygw_get_table_names(GWBUF* querybuf,int* tblsize, bool fullnames) } } + if(tmp != NULL){ + char *catnm = NULL; - char *catnm = NULL; - - if(fullnames) - { - if(tbl->db && strcmp(tbl->db,"skygw_virtual") != 0) - { - catnm = (char*)calloc(strlen(tbl->db) + strlen(tbl->table_name) + 2,sizeof(char)); - strcpy(catnm,tbl->db); - strcat(catnm,"."); - strcat(catnm,tbl->table_name); - } - } + if(fullnames) + { + if(tbl->db && strcmp(tbl->db,"skygw_virtual") != 0) + { + catnm = (char*)calloc(strlen(tbl->db) + strlen(tbl->table_name) + 2,sizeof(char)); + strcpy(catnm,tbl->db); + strcat(catnm,"."); + strcat(catnm,tbl->table_name); + } + } - if(catnm) - { - tables[i++] = catnm; - } - else - { - tables[i++] = strdup(tbl->table_name); - } + if(catnm) + { + tables[i++] = catnm; + } + else + { + tables[i++] = strdup(tbl->table_name); + } - tbl=tbl->next_local; + tbl=tbl->next_local; + } } lex->current_select = lex->current_select->next_select_in_list(); } diff --git a/query_classifier/test/canonical_tests/canonizer.c b/query_classifier/test/canonical_tests/canonizer.c index 1de287015..7983c9041 100644 --- a/query_classifier/test/canonical_tests/canonizer.c +++ b/query_classifier/test/canonical_tests/canonizer.c @@ -57,7 +57,7 @@ int main(int argc, char** argv) { fgets(readbuff,4092,infile); psize = strlen(readbuff); - if(psize < 0 || > 4092){ + if(psize < 0 || psize > 4092){ continue; } qbuff = gwbuf_alloc(psize + 7); diff --git a/server/core/config.c b/server/core/config.c index 1532909d0..f0da45a26 100644 --- a/server/core/config.c +++ b/server/core/config.c @@ -1359,7 +1359,7 @@ SERVER *server; serviceSetUser(obj->element, user, auth); - if (enable_root_user && service) + if (enable_root_user) serviceEnableRootUser(service, atoi(enable_root_user)); if (allow_localhost_match_wildcard_host && service) diff --git a/server/modules/filter/test/harness_common.c b/server/modules/filter/test/harness_common.c index a345a5e09..3c42bfdde 100644 --- a/server/modules/filter/test/harness_common.c +++ b/server/modules/filter/test/harness_common.c @@ -137,8 +137,8 @@ FILTER_PARAMETER** read_params(int* paramc) do_read = 0; } } - FILTER_PARAMETER** params; - if((params = malloc(sizeof(FILTER_PARAMETER*)*(pc+1)))!=NULL){ + FILTER_PARAMETER** params = NULL; + if((params = malloc(sizeof(FILTER_PARAMETER*)*(pc+1))) != NULL){ for(i = 0;irses_prop_data.temp_tables) - { - if (hashtable_delete(rses_prop_tmp->rses_prop_data.temp_tables, - (void *)hkey)) - { - LOGIF(LT, (skygw_log_write(LOGFILE_TRACE, - "Temporary table dropped: %s",hkey))); - } - } - free(tbl[i]); - free(hkey); - } - if(tbl != NULL){ - free(tbl); - } + if (rses_prop_tmp && + rses_prop_tmp->rses_prop_data.temp_tables) + { + if (hashtable_delete(rses_prop_tmp->rses_prop_data.temp_tables, + (void *)hkey)) + { + LOGIF(LT, (skygw_log_write(LOGFILE_TRACE, + "Temporary table dropped: %s",hkey))); + } + } + free(tbl[i]); + free(hkey); + } + + free(tbl); + } } } @@ -1495,7 +1495,7 @@ skygw_query_type_t is_read_tmp_table( { tbl = skygw_get_table_names(querybuf,&tsize,false); - if (tsize > 0) + if (tbl != NULL && tsize > 0) { /** Query targets at least one table */ for(i = 0; i