From 19ab3c1c80fb71df5e620da6d0db94ed3d130d4a Mon Sep 17 00:00:00 2001 From: Guillaume Leconte Date: Thu, 10 Jan 2013 15:12:35 +0100 Subject: [PATCH 1/4] fix: check return values of malloc()/strdup()/realloc() --- haproxy.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 66 insertions(+), 7 deletions(-) diff --git a/haproxy.c b/haproxy.c index 1178c7f..30fc197 100644 --- a/haproxy.c +++ b/haproxy.c @@ -126,6 +126,8 @@ static hap_status_t *hap_status_list = NULL; static int hap_config(const char *key, const char *value) { + char **tmpbuf = NULL; + if (strcasecmp(key, "DisableDeny") == 0) { if IS_TRUE(value) hap_flags &= ~(HAP_DENY); @@ -152,21 +154,45 @@ static int hap_config(const char *key, const char *value) hap_notifs |= HAP_NOTIF_STATUSUP; } else if (strcasecmp(key, "PxFilter") == 0) { pxfcount++; - pxf = realloc(pxf, pxfcount * sizeof(char *)); + tmpbuf = realloc(pxf, pxfcount * sizeof *pxf); + if (! tmpbuf) { + WARNING("realloc(%zu bytes): %s", + pxfcount * sizeof *pxf, strerror(errno)); + return -1; + } + pxf = tmpbuf; pxf[pxfcount - 1] = strdup(value); + if (! pxf[pxfcount -1]) { + WARNING("strdup(%s): %s", value, strerror(errno)); + return -1; + } } else if (strcasecmp(key, "SvFilter") == 0) { svfcount++; - svf = realloc(svf, svfcount * sizeof(char *)); + tmpbuf = realloc(svf, svfcount * sizeof *svf); + if (! tmpbuf) { + WARNING("realloc(%zu bytes): %s", + svfcount * sizeof *svf, strerror(errno)); + return -1; + } + svf = tmpbuf; svf[svfcount - 1] = strdup(value); + if (! svf[svfcount -1]) { + WARNING("strdup(%s): %s", value, strerror(errno)); + return -1; + } } else if (strcasecmp(key, "SocketPath") == 0) { hap_socketpath = strdup(value); + if (! hap_socketpath) { + WARNING("strdup(%s): %s", value, strerror(errno)); + return -1; + } } else if (strcasecmp(key, "RestartGap") == 0) { hap_restart_gap = (unsigned long) atol(value); } else { - return (-1); + return -1; } - return (0); + return 0; } static void hap_submit_counter(const char *svname, const char *pxname, @@ -177,7 +203,11 @@ static void hap_submit_counter(const char *svname, const char *pxname, int i; value_list_t vl = VALUE_LIST_INIT; - values = malloc(len * sizeof(value_t)); + values = malloc(len * sizeof *values); + if (! values) { + WARNING("malloc(%zu bytes): %s", len * sizeof *values, strerror(errno)); + return; + } va_start(ap, len); for (i = 0; i < len; i++) { @@ -222,7 +252,11 @@ static void hap_submit_gauge(const char *svname, const char *pxname, const char int i; value_list_t vl = VALUE_LIST_INIT; - values = malloc(len * sizeof(value_t)); + values = malloc(len * sizeof *values); + if (! values) { + WARNING("malloc(%zu bytes): %s", len * sizeof *values, strerror(errno)); + return; + } va_start(ap, len); for (i = 0; i < len; i++) { @@ -316,10 +350,18 @@ static void hap_line2entry(char *line) case 0: /* filter on proxy */ pentry->pxname = strdup(p); + if (! pentry->pxname) { + WARNING("strdup(%s): %s", p, strerror(errno)); + return; + } break; case 1: /* filter on srv */ pentry->svname = strdup(p); + if (! pentry->svname) { + WARNING("strdup(%s): %s", p, strerror(errno)); + return; + } break; case 4: pentry->stot = (unsigned long long) atoll(p) * interval_g; @@ -603,9 +645,26 @@ hap_status_t *new_hap_status(const char *svname, const char *pxname, if (!svname || !pxname) return NULL; - pstatus = malloc(sizeof(hap_status_t)); + pstatus = malloc(sizeof *pstatus); + if (! pstatus) { + WARNING("malloc(%zu bytes): %s", sizeof *pstatus, strerror(errno)); + return NULL; + } + pstatus->svname = strdup(svname); + if (! pstatus->svname) { + WARNING("strdup(%s): %s", svname, strerror(errno)); + free_hap_status(pstatus); + return NULL; + } + pstatus->pxname = strdup(pxname); + if (! pstatus->pxname) { + WARNING("strdup(%s): %s", pxname, strerror(errno)); + free_hap_status(pstatus); + return NULL; + } + pstatus->status = status; pstatus->next = hap_status_list; From 02cb278d34def1e3a78ec65cabf24ab74f57632b Mon Sep 17 00:00:00 2001 From: Guillaume Leconte Date: Thu, 10 Jan 2013 15:13:00 +0100 Subject: [PATCH 2/4] fix: check return values of fcntl() --- haproxy.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/haproxy.c b/haproxy.c index 30fc197..d01d869 100644 --- a/haproxy.c +++ b/haproxy.c @@ -576,6 +576,11 @@ static int hap_retrievestat(void) strncpy(address.sun_path, hap_socketpath, sizeof(address.sun_path)); arg = fcntl(s_fd, F_GETFL, NULL); + if (-1 == arg) { + WARNING("fcntl: %s", strerror(errno)); + goto err; + } + nbkarg = arg | O_NONBLOCK; fcntl(s_fd, F_SETFL, nbkarg); @@ -589,7 +594,10 @@ static int hap_retrievestat(void) goto err; } - fcntl(s_fd, F_SETFL, arg); + if (-1 == fcntl(s_fd, F_SETFL, arg)) { + WARNING("fcntl: %s", strerror(errno)); + goto err; + } /* retrieve response */ From 6be9250b4211178e4d9ee35d09ffebe7cb5fec27 Mon Sep 17 00:00:00 2001 From: Guillaume Leconte Date: Thu, 10 Jan 2013 15:13:50 +0100 Subject: [PATCH 3/4] new: add a free_hap_status() function for proper cleanup --- haproxy.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/haproxy.c b/haproxy.c index d01d869..4f92e7c 100644 --- a/haproxy.c +++ b/haproxy.c @@ -645,6 +645,17 @@ static int hap_retrievestat(void) return ret; } +static void +free_hap_status(hap_status_t *status) +{ + if (status) { + free(status->svname); + free(status->pxname); + } + + free(status); +} + hap_status_t *new_hap_status(const char *svname, const char *pxname, int status) { From b7475a4ad62c7ea6bda3877cbd06913627f6e390 Mon Sep 17 00:00:00 2001 From: Guillaume Leconte Date: Thu, 10 Jan 2013 15:14:25 +0100 Subject: [PATCH 4/4] fix: check return value of new_hap_status() --- haproxy.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/haproxy.c b/haproxy.c index 4f92e7c..e697d9a 100644 --- a/haproxy.c +++ b/haproxy.c @@ -716,7 +716,7 @@ static int hap_read(void) hap_status_t *pstatus; unsigned long uptime; int i; - + int ret; if (hap_retrievestat() < 0) { goto noreg; @@ -747,7 +747,13 @@ static int hap_read(void) pstatus = get_hap_status(pentry->svname, pentry->pxname); if (!pstatus) { /* insert status to cache */ - new_hap_status(pentry->svname, pentry->pxname, HAP_STATUS_DOWN); + if (NULL == new_hap_status(pentry->svname, + pentry->pxname, + HAP_STATUS_DOWN)) { + WARNING("hap status allocation failed"); + ret = -1; + goto end; + } } else if (pstatus->status != HAP_STATUS_DOWN) { /* notify ? */ if (hap_notifs & HAP_NOTIF_STATUSDOWN) { @@ -761,7 +767,13 @@ static int hap_read(void) /* roll it inverse in UP case */ pstatus = get_hap_status(pentry->svname, pentry->pxname); if (!pstatus) { - new_hap_status(pentry->svname, pentry->pxname, HAP_STATUS_UP); + if (NULL == new_hap_status(pentry->svname, + pentry->pxname, + HAP_STATUS_UP)) { + WARNING("hap status allocation failed"); + ret = -1; + goto end; + } } else if (pstatus->status != HAP_STATUS_UP) { if (hap_notifs & HAP_NOTIF_STATUSUP) { @@ -839,10 +851,11 @@ static int hap_read(void) pfree = pentry; pentry = pentry->next; - if (pfree->pxname) + /* sanity check */ + if (pfree) { free(pfree->pxname); - if (pfree->svname) free(pfree->svname); + } free(pfree); @@ -850,7 +863,9 @@ static int hap_read(void) hap_entry_list = NULL; - return 0; + ret = 0; + end: + return ret; noreg: pentry = hap_entry_list; @@ -860,10 +875,12 @@ static int hap_read(void) pentry = pentry->next; /* free rrd-registered entry */ - if (pfree->pxname) + + /* sanity check here.. */ + if (pfree) { free(pfree->pxname); - if (pfree->svname) free(pfree->svname); + } free(pfree); }