From bbeab80090556ef115b6e11c9bbab534b9ea7717 Mon Sep 17 00:00:00 2001 From: Justin Brewer Date: Thu, 12 Apr 2018 15:22:51 -0500 Subject: [PATCH 1/6] Use AF_UNIX AF_LOCAL is the old, non-standardized name for AF_UNIX. Just use AF_UNIX, rather than wrestling with platform specifics of AF_LOCAL definitions. Signed-off-by: Justin Brewer --- net.c | 4 ++-- net.h | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/net.c b/net.c index 33d9a82..99f647d 100644 --- a/net.c +++ b/net.c @@ -432,7 +432,7 @@ int redisContextConnectUnix(redisContext *c, const char *path, const struct time struct sockaddr_un sa; long timeout_msec = -1; - if (redisCreateSocket(c,AF_LOCAL) < 0) + if (redisCreateSocket(c,AF_UNIX) < 0) return REDIS_ERR; if (redisSetBlocking(c,0) != REDIS_OK) return REDIS_ERR; @@ -457,7 +457,7 @@ int redisContextConnectUnix(redisContext *c, const char *path, const struct time if (redisContextTimeoutMsec(c,&timeout_msec) != REDIS_OK) return REDIS_ERR; - sa.sun_family = AF_LOCAL; + sa.sun_family = AF_UNIX; strncpy(sa.sun_path,path,sizeof(sa.sun_path)-1); if (connect(c->fd, (struct sockaddr*)&sa, sizeof(sa)) == -1) { if (errno == EINPROGRESS && !blocking) { diff --git a/net.h b/net.h index fc8ddd3..d9dc362 100644 --- a/net.h +++ b/net.h @@ -37,10 +37,6 @@ #include "hiredis.h" -#if defined(__sun) || defined(AIX) -#define AF_LOCAL AF_UNIX -#endif - int redisCheckSocketError(redisContext *c); int redisContextSetTimeout(redisContext *c, const struct timeval tv); int redisContextConnectTcp(redisContext *c, const char *addr, int port, const struct timeval *timeout); From 49bbaacc79dc73ed797450aa94463106b18d2a5d Mon Sep 17 00:00:00 2001 From: Justin Brewer Date: Fri, 13 Apr 2018 14:40:34 -0500 Subject: [PATCH 2/6] Strip down fmacros.h strerror_r and addrinfo require _POSIX_C_SOURCE >= 200112L, which is implied by _XOPEN_SOURCE >= 600. With the removal of AF_LOCAL usage, the only non-standard features being used are the TCP_KEEP* socket flags. _DARWIN_C_SOURCE is required to expose TCP_KEEPALIVE. Fall back to using _XOPEN_SOURCE 600 for all platforms, and additionally define _DARWIN_C_SOURCE for Darwin. Signed-off-by: Justin Brewer --- fmacros.h | 30 ++---------------------------- net.c | 2 +- 2 files changed, 3 insertions(+), 29 deletions(-) diff --git a/fmacros.h b/fmacros.h index 4cdbc13..3227faa 100644 --- a/fmacros.h +++ b/fmacros.h @@ -1,38 +1,12 @@ #ifndef __HIREDIS_FMACRO_H #define __HIREDIS_FMACRO_H -#if defined(__linux__) -#define _BSD_SOURCE -#define _DEFAULT_SOURCE -#endif - -#if defined(__CYGWIN__) -#include -#endif - -#if defined(__linux__) || defined(__OpenBSD__) || defined(__NetBSD__) #define _XOPEN_SOURCE 600 -#elif defined(__APPLE__) && defined(__MACH__) -#define _XOPEN_SOURCE -#elif defined(__FreeBSD__) -// intentionally left blank, don't define _XOPEN_SOURCE -#elif defined(AIX) -// intentionally left blank, don't define _XOPEN_SOURCE -#else -#define _XOPEN_SOURCE -#endif - -#if defined(__sun__) #define _POSIX_C_SOURCE 200112L -#endif #if defined(__APPLE__) && defined(__MACH__) -#define _OSX -#endif - -#ifndef AIX -# define _XOPEN_SOURCE_EXTENDED 1 -# define _ALL_SOURCE +/* Enable TCP_KEEPALIVE */ +#define _DARWIN_C_SOURCE #endif #endif diff --git a/net.c b/net.c index 99f647d..44f2575 100644 --- a/net.c +++ b/net.c @@ -136,7 +136,7 @@ int redisKeepAlive(redisContext *c, int interval) { val = interval; -#ifdef _OSX +#if defined(__APPLE__) && defined(__MACH__) if (setsockopt(fd, IPPROTO_TCP, TCP_KEEPALIVE, &val, sizeof(val)) < 0) { __redisSetError(c,REDIS_ERR_OTHER,strerror(errno)); return REDIS_ERR; From d1c1b668c14dced57026aca936a57c2567f65107 Mon Sep 17 00:00:00 2001 From: Justin Brewer Date: Tue, 17 Apr 2018 15:04:37 -0500 Subject: [PATCH 3/6] Drop __redis_strerror_r Since _GNU_SOURCE is now guaranteed to be unset, it is no longer necessary to support the GNU-specific version of strerror_r. Drop __redis_strerror_r from the header, and call strerror_r directly. This breaks any external users of this macro, but they shouldn't have been using it in the first place. Signed-off-by: Justin Brewer --- hiredis.c | 2 +- hiredis.h | 24 ------------------------ net.c | 2 +- 3 files changed, 2 insertions(+), 26 deletions(-) diff --git a/hiredis.c b/hiredis.c index b344962..f119d50 100644 --- a/hiredis.c +++ b/hiredis.c @@ -581,7 +581,7 @@ void __redisSetError(redisContext *c, int type, const char *str) { } else { /* Only REDIS_ERR_IO may lack a description! */ assert(type == REDIS_ERR_IO); - __redis_strerror_r(errno, c->errstr, sizeof(c->errstr)); + strerror_r(errno, c->errstr, sizeof(c->errstr)); } } diff --git a/hiredis.h b/hiredis.h index 77d5797..a743760 100644 --- a/hiredis.h +++ b/hiredis.h @@ -80,30 +80,6 @@ * SO_REUSEADDR is being used. */ #define REDIS_CONNECT_RETRIES 10 -/* strerror_r has two completely different prototypes and behaviors - * depending on system issues, so we need to operate on the error buffer - * differently depending on which strerror_r we're using. */ -#ifndef _GNU_SOURCE -/* "regular" POSIX strerror_r that does the right thing. */ -#define __redis_strerror_r(errno, buf, len) \ - do { \ - strerror_r((errno), (buf), (len)); \ - } while (0) -#else -/* "bad" GNU strerror_r we need to clean up after. */ -#define __redis_strerror_r(errno, buf, len) \ - do { \ - char *err_str = strerror_r((errno), (buf), (len)); \ - /* If return value _isn't_ the start of the buffer we passed in, \ - * then GNU strerror_r returned an internal static buffer and we \ - * need to copy the result into our private buffer. */ \ - if (err_str != (buf)) { \ - strncpy((buf), err_str, ((len) - 1)); \ - (buf)[(len)-1] = '\0'; \ - } \ - } while (0) -#endif - #ifdef __cplusplus extern "C" { #endif diff --git a/net.c b/net.c index 44f2575..4551f3e 100644 --- a/net.c +++ b/net.c @@ -71,7 +71,7 @@ static void __redisSetErrorFromErrno(redisContext *c, int type, const char *pref if (prefix != NULL) len = snprintf(buf,sizeof(buf),"%s: ",prefix); - __redis_strerror_r(errorno, (char *)(buf + len), sizeof(buf) - len); + strerror_r(errorno, (char *)(buf + len), sizeof(buf) - len); __redisSetError(c,type,buf); } From 546d9415e1806565b6c8f413c8873726c6aa049f Mon Sep 17 00:00:00 2001 From: Justin Brewer Date: Tue, 17 Apr 2018 13:47:51 -0500 Subject: [PATCH 4/6] Fix a segfault on *BSD freeaddrinfo is not required by POSIX to be NULL-safe. OpenBSD will SIGSEGV. NetBSD will assert. FreeBSD up to 11.1 will SIGSEGV, while in future versions, it will be a silent NOP [1]. Commit d4b715f0aa97 ("Fix potential race in 'invalid timeout' tests") added a code path to _redisContextConnectTcp which calls freeaddrinfo(NULL), triggering the segfault. Put a NULL check around the call to freeaddrinfo. [1] - https://github.com/freebsd/freebsd/commit/e9167239034a1e475c3238f8d133ebf703424ee0 Signed-off-by: Justin Brewer --- net.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net.c b/net.c index 4551f3e..7ce9ef4 100644 --- a/net.c +++ b/net.c @@ -412,7 +412,10 @@ addrretry: error: rv = REDIS_ERR; end: - freeaddrinfo(servinfo); + if(servinfo) { + freeaddrinfo(servinfo); + } + return rv; // Need to return REDIS_OK if alright } From 54acc8f08759713338b6cb6c94916438bbd91b80 Mon Sep 17 00:00:00 2001 From: Justin Brewer Date: Tue, 27 Mar 2018 13:44:01 -0500 Subject: [PATCH 5/6] Remove redundant zero stores calloc is guaranteed to provide a zero-initialized buffer. There is no need to set fields to zero explicitly. Signed-off-by: Justin Brewer --- hiredis.c | 6 ------ read.c | 2 -- 2 files changed, 8 deletions(-) diff --git a/hiredis.c b/hiredis.c index f119d50..8eabac0 100644 --- a/hiredis.c +++ b/hiredis.c @@ -596,14 +596,8 @@ static redisContext *redisContextInit(void) { if (c == NULL) return NULL; - c->err = 0; - c->errstr[0] = '\0'; c->obuf = sdsempty(); c->reader = redisReaderCreate(); - c->tcp.host = NULL; - c->tcp.source_addr = NULL; - c->unix_sock.path = NULL; - c->timeout = NULL; if (c->obuf == NULL || c->reader == NULL) { redisFree(c); diff --git a/read.c b/read.c index 061bbda..4baa74e 100644 --- a/read.c +++ b/read.c @@ -420,8 +420,6 @@ redisReader *redisReaderCreateWithFunctions(redisReplyObjectFunctions *fn) { if (r == NULL) return NULL; - r->err = 0; - r->errstr[0] = '\0'; r->fn = fn; r->buf = sdsempty(); r->maxbuf = REDIS_READER_MAX_BUF; From 58e6b87f51ba31b4a2954d0babe6b3cf7019c985 Mon Sep 17 00:00:00 2001 From: Justin Brewer Date: Tue, 27 Mar 2018 13:47:29 -0500 Subject: [PATCH 6/6] Remove redundant NULL checks free(NULL) is a valid NOP. Most of the hiredis free functions behave the same way. redisReaderFree is updated to also be NULL-safe. There is one redundant NULL check at sds.c:1036, but it's left as is since sds is imported from upstream. Signed-off-by: Justin Brewer --- hiredis.c | 30 +++++++++--------------------- net.c | 9 +++------ read.c | 13 ++++++------- 3 files changed, 18 insertions(+), 34 deletions(-) diff --git a/hiredis.c b/hiredis.c index 8eabac0..98f43c9 100644 --- a/hiredis.c +++ b/hiredis.c @@ -84,16 +84,14 @@ void freeReplyObject(void *reply) { case REDIS_REPLY_ARRAY: if (r->element != NULL) { for (j = 0; j < r->elements; j++) - if (r->element[j] != NULL) - freeReplyObject(r->element[j]); + freeReplyObject(r->element[j]); free(r->element); } break; case REDIS_REPLY_ERROR: case REDIS_REPLY_STATUS: case REDIS_REPLY_STRING: - if (r->str != NULL) - free(r->str); + free(r->str); break; } free(r); @@ -432,11 +430,7 @@ cleanup: } sdsfree(curarg); - - /* No need to check cmd since it is the last statement that can fail, - * but do it anyway to be as defensive as possible. */ - if (cmd != NULL) - free(cmd); + free(cmd); return error_type; } @@ -612,18 +606,12 @@ void redisFree(redisContext *c) { return; if (c->fd > 0) close(c->fd); - if (c->obuf != NULL) - sdsfree(c->obuf); - if (c->reader != NULL) - redisReaderFree(c->reader); - if (c->tcp.host) - free(c->tcp.host); - if (c->tcp.source_addr) - free(c->tcp.source_addr); - if (c->unix_sock.path) - free(c->unix_sock.path); - if (c->timeout) - free(c->timeout); + sdsfree(c->obuf); + redisReaderFree(c->reader); + free(c->tcp.host); + free(c->tcp.source_addr); + free(c->unix_sock.path); + free(c->timeout); free(c); } diff --git a/net.c b/net.c index 7ce9ef4..5d50f7c 100644 --- a/net.c +++ b/net.c @@ -285,8 +285,7 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port, * This is a bit ugly, but atleast it works and doesn't leak memory. **/ if (c->tcp.host != addr) { - if (c->tcp.host) - free(c->tcp.host); + free(c->tcp.host); c->tcp.host = strdup(addr); } @@ -299,8 +298,7 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port, memcpy(c->timeout, timeout, sizeof(struct timeval)); } } else { - if (c->timeout) - free(c->timeout); + free(c->timeout); c->timeout = NULL; } @@ -452,8 +450,7 @@ int redisContextConnectUnix(redisContext *c, const char *path, const struct time memcpy(c->timeout, timeout, sizeof(struct timeval)); } } else { - if (c->timeout) - free(c->timeout); + free(c->timeout); c->timeout = NULL; } diff --git a/read.c b/read.c index 4baa74e..2bad85e 100644 --- a/read.c +++ b/read.c @@ -52,11 +52,9 @@ static void __redisReaderSetError(redisReader *r, int type, const char *str) { } /* Clear input buffer on errors. */ - if (r->buf != NULL) { - sdsfree(r->buf); - r->buf = NULL; - r->pos = r->len = 0; - } + sdsfree(r->buf); + r->buf = NULL; + r->pos = r->len = 0; /* Reset task stack. */ r->ridx = -1; @@ -433,10 +431,11 @@ redisReader *redisReaderCreateWithFunctions(redisReplyObjectFunctions *fn) { } void redisReaderFree(redisReader *r) { + if (r == NULL) + return; if (r->reply != NULL && r->fn && r->fn->freeObject) r->fn->freeObject(r->reply); - if (r->buf != NULL) - sdsfree(r->buf); + sdsfree(r->buf); free(r); }