From 148781210fbcb1b26d5df678a400fa5dd8549e42 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Fri, 10 Feb 2017 15:04:06 +0100 Subject: [PATCH 1/2] lib: fix remaining coverity issues Reported-by: Coverity Signed-off-by: David Lamparter --- lib/command.c | 9 ++++++--- lib/csv.c | 6 +++++- lib/grammar_sandbox.c | 16 +++++++++++++-- lib/mpls.h | 45 +++++++++++++------------------------------ lib/skiplist.c | 3 ++- lib/vty.c | 2 +- lib/zclient.c | 5 +++-- 7 files changed, 44 insertions(+), 42 deletions(-) diff --git a/lib/command.c b/lib/command.c index 4a84386858..d1e11f0617 100644 --- a/lib/command.c +++ b/lib/command.c @@ -1450,7 +1450,8 @@ DEFUN (config_write, VTY_NEWLINE); goto finished; } - fsync (dirfd); + if (dirfd >= 0) + fsync (dirfd); } if (rename (config_file_tmp, config_file) != 0) { @@ -1458,7 +1459,8 @@ DEFUN (config_write, VTY_NEWLINE); goto finished; } - fsync (dirfd); + if (dirfd >= 0) + fsync (dirfd); vty_out (vty, "Configuration saved to %s%s", config_file, VTY_NEWLINE); @@ -1467,7 +1469,8 @@ DEFUN (config_write, finished: if (ret != CMD_SUCCESS) unlink (config_file_tmp); - close (dirfd); + if (dirfd >= 0) + close (dirfd); XFREE (MTYPE_TMP, config_file_tmp); XFREE (MTYPE_TMP, config_file_sav); return ret; diff --git a/lib/csv.c b/lib/csv.c index 4fd14918fd..d614ac3066 100644 --- a/lib/csv.c +++ b/lib/csv.c @@ -177,8 +177,11 @@ csv_decode_record(csv_record_t *rec) field = strpbrk(curr, ","); } field = strstr(curr, "\n"); + if (!field) + return; + fld = malloc(sizeof(csv_field_t)); - if (field && fld) { + if (fld) { fld->field = curr; fld->field_len = field-curr; TAILQ_INSERT_TAIL(&(rec->fields), fld, next_field); @@ -420,6 +423,7 @@ csv_clone_record (csv_t *csv, csv_record_t *in_rec, csv_record_t **out_rec) curr = calloc(1, csv->buflen); if (!curr) { log_error("field str malloc failed\n"); + free(rec); return; } rec->record = curr; diff --git a/lib/grammar_sandbox.c b/lib/grammar_sandbox.c index 315bd4d59c..08f012f8ac 100644 --- a/lib/grammar_sandbox.c +++ b/lib/grammar_sandbox.c @@ -86,6 +86,11 @@ DEFUN (grammar_test_complete, return CMD_SUCCESS; vector command = cmd_make_strvec (cmdstr); + if (!command) + { + XFREE (MTYPE_TMP, cmdstr); + return CMD_SUCCESS; + } // generate completions of user input struct list *completions; @@ -121,7 +126,7 @@ DEFUN (grammar_test_complete, // free resources list_delete (completions); cmd_free_strvec (command); - free (cmdstr); + XFREE (MTYPE_TMP, cmdstr); return CMD_SUCCESS; } @@ -138,7 +143,14 @@ DEFUN (grammar_test_match, return CMD_SUCCESS; char *cmdstr = argv_concat(argv, argc, idx_command); + if (!cmdstr) + return CMD_SUCCESS; vector command = cmd_make_strvec (cmdstr); + if (!command) + { + XFREE (MTYPE_TMP, cmdstr); + return CMD_SUCCESS; + } struct list *argvv = NULL; const struct cmd_element *element = NULL; @@ -177,7 +189,7 @@ DEFUN (grammar_test_match, // free resources cmd_free_strvec (command); - free (cmdstr); + XFREE (MTYPE_TMP, cmdstr); return CMD_SUCCESS; } diff --git a/lib/mpls.h b/lib/mpls.h index 1f77aaa536..b5c1a653b1 100644 --- a/lib/mpls.h +++ b/lib/mpls.h @@ -123,59 +123,40 @@ mpls_lse_decode (mpls_lse_t lse, mpls_label_t *label, /* Printable string for labels (with consideration for reserved values). */ static inline char * -label2str (mpls_label_t label, char *buf, int len) +label2str (mpls_label_t label, char *buf, size_t len) { switch(label) { case MPLS_V4_EXP_NULL_LABEL: - strncpy(buf, "IPv4 Explicit Null", len); + strlcpy(buf, "IPv4 Explicit Null", len); return(buf); - break; case MPLS_RA_LABEL: - strncpy(buf, "Router Alert", len); + strlcpy(buf, "Router Alert", len); return(buf); - break; case MPLS_V6_EXP_NULL_LABEL: - strncpy(buf, "IPv6 Explict Null", len); + strlcpy(buf, "IPv6 Explict Null", len); return(buf); - break; case MPLS_IMP_NULL_LABEL: - strncpy(buf, "implicit-null", len); + strlcpy(buf, "implicit-null", len); return(buf); - break; case MPLS_ENTROPY_LABEL_INDICATOR: - strncpy(buf, "Entropy Label Indicator", len); + strlcpy(buf, "Entropy Label Indicator", len); return(buf); - break; case MPLS_GAL_LABEL: - strncpy(buf, "Generic Associated Channel", len); + strlcpy(buf, "Generic Associated Channel", len); return(buf); - break; case MPLS_OAM_ALERT_LABEL: - strncpy(buf, "OAM Alert", len); + strlcpy(buf, "OAM Alert", len); return(buf); - break; case MPLS_EXTENSION_LABEL: - strncpy(buf, "Extension", len); + strlcpy(buf, "Extension", len); return(buf); - break; - case 4: - case 5: - case 6: - case 8: - case 9: - case 10: - case 11: - case 12: - strncpy(buf, "Reserved", len); - return(buf); - break; default: - sprintf(buf, "%u", label); + if (label < 16) + snprintf(buf, len, "Reserved (%u)", label); + else + snprintf(buf, len, "%u", label); return(buf); } - - strncpy(buf, "Error", len); - return(buf); } /* constants used by ldpd */ diff --git a/lib/skiplist.c b/lib/skiplist.c index 2a90b2c7c6..05f489c905 100644 --- a/lib/skiplist.c +++ b/lib/skiplist.c @@ -439,7 +439,8 @@ skiplist_next_value( return -1; *valuePointer = q->value; - *cursor = q; + if (cursor) + *cursor = q; CHECKLAST(l); return 0; } diff --git a/lib/vty.c b/lib/vty.c index 0568351989..7983e67a55 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -1993,7 +1993,7 @@ vty_serv_un (const char *path) /* Make server socket. */ memset (&serv, 0, sizeof (struct sockaddr_un)); serv.sun_family = AF_UNIX; - strncpy (serv.sun_path, path, strlen (path)); + strlcpy (serv.sun_path, path, sizeof (serv.sun_path)); #ifdef HAVE_STRUCT_SOCKADDR_UN_SUN_LEN len = serv.sun_len = SUN_LEN(&serv); #else diff --git a/lib/zclient.c b/lib/zclient.c index cea4b098fc..859751deb8 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -35,6 +35,7 @@ #include "nexthop.h" DEFINE_MTYPE_STATIC(LIB, ZCLIENT, "Zclient") +DEFINE_MTYPE_STATIC(LIB, REDIST_INST, "Redistribution instance IDs") /* Zebra client events. */ enum event {ZCLIENT_SCHEDULE, ZCLIENT_READ, ZCLIENT_CONNECT}; @@ -106,7 +107,7 @@ redist_add_instance (struct redist_proto *red, u_short instance) if (!red->instances) red->instances = list_new(); - in = calloc (1, sizeof(u_short)); + in = XMALLOC (MTYPE_REDIST_INST, sizeof(u_short)); *in = instance; listnode_add (red->instances, in); } @@ -121,7 +122,7 @@ redist_del_instance (struct redist_proto *red, u_short instance) return; listnode_delete(red->instances, id); - free (id); + XFREE (MTYPE_REDIST_INST, id); if (!red->instances->count) { red->enabled = 0; From 6de469061b9933be7afaf2712cb45cb13c51a90c Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Fri, 10 Feb 2017 15:04:40 +0100 Subject: [PATCH 2/2] lib: rework vty_use_backup_config() Like config_write(), this should use rename(), even though atomicity is not a real issue here. Signed-off-by: David Lamparter --- lib/vty.c | 50 ++++++++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/lib/vty.c b/lib/vty.c index 7983e67a55..ce6349bf77 100644 --- a/lib/vty.c +++ b/lib/vty.c @@ -2357,7 +2357,6 @@ vty_use_backup_config (char *fullpath) { char *fullpath_sav, *fullpath_tmp; FILE *ret = NULL; - struct stat buf; int tmp, sav; int c; char buffer[512]; @@ -2365,7 +2364,9 @@ vty_use_backup_config (char *fullpath) fullpath_sav = malloc (strlen (fullpath) + strlen (CONF_BACKUP_EXT) + 1); strcpy (fullpath_sav, fullpath); strcat (fullpath_sav, CONF_BACKUP_EXT); - if (stat (fullpath_sav, &buf) == -1) + + sav = open (fullpath_sav, O_RDONLY); + if (sav < 0) { free (fullpath_sav); return NULL; @@ -2377,47 +2378,32 @@ vty_use_backup_config (char *fullpath) /* Open file to configuration write. */ tmp = mkstemp (fullpath_tmp); if (tmp < 0) - { - free (fullpath_sav); - free (fullpath_tmp); - return NULL; - } + goto out_close_sav; - sav = open (fullpath_sav, O_RDONLY); - if (sav < 0) - { - unlink (fullpath_tmp); - free (fullpath_sav); - free (fullpath_tmp); - return NULL; - } + if (fchmod (tmp, CONFIGFILE_MASK) != 0) + goto out_close; while((c = read (sav, buffer, 512)) > 0) { if (write (tmp, buffer, c) <= 0) - { - free (fullpath_sav); - free (fullpath_tmp); - close (sav); - close (tmp); - return NULL; - } + goto out_close; } close (sav); close (tmp); - if (chmod(fullpath_tmp, CONFIGFILE_MASK) != 0) - { - unlink (fullpath_tmp); - free (fullpath_sav); - free (fullpath_tmp); - return NULL; - } - - if (link (fullpath_tmp, fullpath) == 0) + if (rename (fullpath_tmp, fullpath) == 0) ret = fopen (fullpath, "r"); + else + unlink (fullpath_tmp); - unlink (fullpath_tmp); + if (0) + { +out_close: + close (tmp); + unlink (fullpath_tmp); +out_close_sav: + close (sav); + } free (fullpath_sav); free (fullpath_tmp);