Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sign error, int/size_t discrepancies, warnings on windows builds. #936

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions alloc.c
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*/

/* turn of windows warnings for _strcmp etc. */
#define _CRT_NONSTDC_NO_DEPRECATE

#include "fmacros.h"
#include "alloc.h"
Expand Down
5 changes: 4 additions & 1 deletion async.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
* POSSIBILITY OF SUCH DAMAGE.
*/

/* turn of windows warnings for _strcmp etc. */
#define _CRT_NONSTDC_NO_DEPRECATE

#include "fmacros.h"
#include "alloc.h"
#include <stdlib.h>
Expand Down Expand Up @@ -75,7 +78,7 @@ static void *callbackValDup(void *privdata, const void *src) {
}

static int callbackKeyCompare(void *privdata, const void *key1, const void *key2) {
int l1, l2;
size_t l1, l2;
((void) privdata);

l1 = sdslen((const sds)key1);
Expand Down
2 changes: 1 addition & 1 deletion dict.c
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ static int _dictInit(dict *ht, dictType *type, void *privDataPtr);

/* Generic hash function (a popular one from Bernstein).
* I tested a few and this was the best. */
static unsigned int dictGenHashFunction(const unsigned char *buf, int len) {
static unsigned int dictGenHashFunction(const unsigned char *buf, size_t len) {
unsigned int hash = 5381;

while (len--)
Expand Down
2 changes: 1 addition & 1 deletion dict.h
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ typedef struct dictIterator {
#define dictSize(ht) ((ht)->used)

/* API */
static unsigned int dictGenHashFunction(const unsigned char *buf, int len);
static unsigned int dictGenHashFunction(const unsigned char *buf, size_t len);
static dict *dictCreate(dictType *type, void *privDataPtr);
static int dictExpand(dict *ht, unsigned long size);
static int dictAdd(dict *ht, void *key, void *val);
Expand Down
16 changes: 8 additions & 8 deletions hiredis.c
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,12 @@ static size_t bulklen(size_t len) {
int redisvFormatCommand(char **target, const char *format, va_list ap) {
const char *c = format;
char *cmd = NULL; /* final command */
int pos; /* position in final command */
size_t pos; /* position in final command */
sds curarg, newarg; /* current argument */
int touched = 0; /* was the current argument touched? */
char **curargv = NULL, **newargv = NULL;
int argc = 0;
int totlen = 0;
size_t totlen = 0;
int error_type = 0; /* 0 = no error; -1 = memory error; -2 = format error */
int j;

Expand Down Expand Up @@ -516,7 +516,7 @@ int redisvFormatCommand(char **target, const char *format, va_list ap) {

hi_free(curargv);
*target = cmd;
return totlen;
return (int)totlen;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands really should return a ssize_t result to be consistent with the use of size_t, but I didn't want to modify their signature since they are a public API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since then, have updated to reflect the new api which returns long long. Why that was chosen instead of size_t or ssize_t is unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands really should return a ssize_t result to be consistent with the use of size_t, but I didn't want to modify their signature since they are a public API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this comment was written, the API has been updated to return long long. Why that was chosen instead of the more standard ssize_t remains unclear.


format_err:
error_type = -2;
Expand Down Expand Up @@ -576,7 +576,7 @@ long long redisFormatSdsCommandArgv(sds *target, int argc, const char **argv,
const size_t *argvlen)
{
sds cmd, aux;
unsigned long long totlen, len;
size_t totlen, len;
int j;

/* Abort on a NULL target */
Expand Down Expand Up @@ -608,15 +608,15 @@ long long redisFormatSdsCommandArgv(sds *target, int argc, const char **argv,
cmd = sdscatfmt(cmd, "*%i\r\n", argc);
for (j=0; j < argc; j++) {
len = argvlen ? argvlen[j] : strlen(argv[j]);
cmd = sdscatfmt(cmd, "$%U\r\n", len);
cmd = sdscatfmt(cmd, "$%U\r\n", (unsigned long long)len);
cmd = sdscatlen(cmd, argv[j], len);
cmd = sdscatlen(cmd, "\r\n", sizeof("\r\n")-1);
}

assert(sdslen(cmd)==totlen);

*target = cmd;
return totlen;
return (long long) totlen; /* api doesn't use ssize_t */
}

void redisFreeSdsCommand(sds cmd) {
Expand Down Expand Up @@ -663,7 +663,7 @@ long long redisFormatCommandArgv(char **target, int argc, const char **argv, con
cmd[pos] = '\0';

*target = cmd;
return totlen;
return (long long) totlen; /* api doesn't use ssize_t */
}

void redisFreeCommand(char *cmd) {
Expand Down Expand Up @@ -941,7 +941,7 @@ redisPushFn *redisSetPushCallback(redisContext *c, redisPushFn *fn) {
* see if there is a reply available. */
int redisBufferRead(redisContext *c) {
char buf[1024*16];
int nread;
ssize_t nread;

/* Return early when the context has seen an error. */
if (c->err)
Expand Down
14 changes: 7 additions & 7 deletions net.c
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ static int redisContextWaitReady(redisContext *c, long msec) {
}

int redisCheckConnectDone(redisContext *c, int *completed) {
int rc = connect(c->fd, (const struct sockaddr *)c->saddr, c->addrlen);
int rc = connect(c->fd, (const struct sockaddr *)c->saddr, (socklen_t)c->addrlen);
if (rc == 0) {
*completed = 1;
return REDIS_OK;
Expand Down Expand Up @@ -331,7 +331,7 @@ int redisCheckSocketError(redisContext *c) {

int redisContextSetTimeout(redisContext *c, const struct timeval tv) {
const void *to_ptr = &tv;
size_t to_sz = sizeof(tv);
socklen_t to_sz = sizeof(tv);

if (setsockopt(c->fd,SOL_SOCKET,SO_RCVTIMEO,to_ptr,to_sz) == -1) {
__redisSetErrorFromErrno(c,REDIS_ERR_IO,"setsockopt(SO_RCVTIMEO)");
Expand Down Expand Up @@ -473,7 +473,7 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
}

for (b = bservinfo; b != NULL; b = b->ai_next) {
if (bind(s,b->ai_addr,b->ai_addrlen) != -1) {
if (bind(s,b->ai_addr,(socklen_t)b->ai_addrlen) != -1) {
Copy link
Contributor Author

@kristjanvalur kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows has ai_addren as size_t rather than socklen_t. explicitly cast it to silence warning.

bound = 1;
break;
}
Expand All @@ -496,7 +496,7 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
memcpy(c->saddr, p->ai_addr, p->ai_addrlen);
c->addrlen = p->ai_addrlen;

if (connect(s,p->ai_addr,p->ai_addrlen) == -1) {
if (connect(s,p->ai_addr,(socklen_t)p->ai_addrlen) == -1) {
if (errno == EHOSTUNREACH) {
redisNetClose(c);
continue;
Expand Down Expand Up @@ -616,13 +616,13 @@ int redisContextConnectUnix(redisContext *c, const char *path, const struct time

c->flags |= REDIS_CONNECTED;
return REDIS_OK;
oom:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix error about unused label (windows)

__redisSetError(c, REDIS_ERR_OOM, "Out of memory");
return REDIS_ERR;
#else
/* We currently do not support Unix sockets for Windows. */
/* TODO(m): https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/ */
errno = EPROTONOSUPPORT;
return REDIS_ERR;
#endif /* _WIN32 */
oom:
__redisSetError(c, REDIS_ERR_OOM, "Out of memory");
return REDIS_ERR;
}
22 changes: 13 additions & 9 deletions read.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
* POSSIBILITY OF SUCH DAMAGE.
*/

/* turn of windows warnings for _strcmp etc. */
#define _CRT_NONSTDC_NO_DEPRECATE

#include "fmacros.h"
#include <string.h>
#include <stdlib.h>
Expand Down Expand Up @@ -213,7 +216,7 @@ static int string2ll(const char *s, size_t slen, long long *value) {
if (negative) {
if (v > ((unsigned long long)(-(LLONG_MIN+1))+1)) /* Overflow. */
return REDIS_ERR;
if (value != NULL) *value = -v;
if (value != NULL) *value = -(long long)v;
Copy link
Contributor Author

@kristjanvalur kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning on windows. Negation of an unsigned variable is shaky ground in C, better to be explicit.

} else {
if (v > LLONG_MAX) /* Overflow. */
return REDIS_ERR;
Expand All @@ -222,9 +225,9 @@ static int string2ll(const char *s, size_t slen, long long *value) {
return REDIS_OK;
}

static char *readLine(redisReader *r, int *_len) {
static char *readLine(redisReader *r, size_t *_len) {
char *p, *s;
int len;
size_t len;

p = r->buf+r->pos;
s = seekNewline(p,(r->len-r->pos));
Expand Down Expand Up @@ -269,7 +272,7 @@ static int processLineItem(redisReader *r) {
redisReadTask *cur = r->task[r->ridx];
void *obj;
char *p;
int len;
size_t len;

if ((p = readLine(r,&len)) != NULL) {
if (cur->type == REDIS_REPLY_INTEGER) {
Expand All @@ -290,7 +293,7 @@ static int processLineItem(redisReader *r) {
char buf[326], *eptr;
double d;

if ((size_t)len >= sizeof(buf)) {
if (len >= sizeof(buf)) {
__redisReaderSetError(r,REDIS_ERR_PROTOCOL,
"Double value is too large");
return REDIS_ERR;
Expand Down Expand Up @@ -349,7 +352,7 @@ static int processLineItem(redisReader *r) {
} else if (cur->type == REDIS_REPLY_BIGNUM) {
/* Ensure all characters are decimal digits (with possible leading
* minus sign). */
for (int i = 0; i < len; i++) {
for (size_t i = 0; i < len; i++) {
/* XXX Consider: Allow leading '+'? Error on leading '0's? */
if (i == 0 && p[0] == '-') continue;
if (p[i] < '0' || p[i] > '9') {
Expand All @@ -364,7 +367,7 @@ static int processLineItem(redisReader *r) {
obj = (void*)REDIS_REPLY_BIGNUM;
} else {
/* Type will be error or status. */
for (int i = 0; i < len; i++) {
for (size_t i = 0; i < len; i++) {
if (p[i] == '\r' || p[i] == '\n') {
__redisReaderSetError(r,REDIS_ERR_PROTOCOL,
"Bad simple string value");
Expand Down Expand Up @@ -396,7 +399,7 @@ static int processBulkItem(redisReader *r) {
void *obj = NULL;
char *p, *s;
long long len;
unsigned long bytelen;
size_t bytelen;
int success = 0;

p = r->buf+r->pos;
Expand Down Expand Up @@ -494,7 +497,8 @@ static int processAggregateItem(redisReader *r) {
void *obj;
char *p;
long long elements;
int root = 0, len;
int root = 0;
size_t len;

if (r->ridx == r->tasks - 1) {
if (redisReaderGrow(r) == REDIS_ERR)
Expand Down
24 changes: 12 additions & 12 deletions sds.c
Original file line number Diff line number Diff line change
Expand Up @@ -99,27 +99,27 @@ sds sdsnewlen(const void *init, size_t initlen) {
fp = ((unsigned char*)s)-1;
switch(type) {
case SDS_TYPE_5: {
*fp = type | (initlen << SDS_TYPE_BITS);
*fp = type | ((char)initlen << SDS_TYPE_BITS);
break;
}
case SDS_TYPE_8: {
SDS_HDR_VAR(8,s);
sh->len = initlen;
sh->alloc = initlen;
sh->len = (uint8_t) initlen;
sh->alloc = (uint8_t) initlen;
*fp = type;
break;
}
case SDS_TYPE_16: {
SDS_HDR_VAR(16,s);
sh->len = initlen;
sh->alloc = initlen;
sh->len = (uint16_t)initlen;
sh->alloc = (uint16_t)initlen;
*fp = type;
break;
}
case SDS_TYPE_32: {
SDS_HDR_VAR(32,s);
sh->len = initlen;
sh->alloc = initlen;
sh->len = (uint32_t)initlen;
sh->alloc = (uint32_t)initlen;
*fp = type;
break;
}
Expand Down Expand Up @@ -431,7 +431,7 @@ sds sdscpy(sds s, const char *t) {
* The function returns the length of the null-terminated string
* representation stored at 's'. */
#define SDS_LLSTR_SIZE 21
int sdsll2str(char *s, long long value) {
size_t sdsll2str(char *s, long long value) {
char *p, aux;
unsigned long long v;
size_t l;
Expand Down Expand Up @@ -463,7 +463,7 @@ int sdsll2str(char *s, long long value) {
}

/* Identical sdsll2str(), but for unsigned long long type. */
int sdsull2str(char *s, unsigned long long v) {
size_t sdsull2str(char *s, unsigned long long v) {
char *p, aux;
size_t l;

Expand Down Expand Up @@ -497,7 +497,7 @@ int sdsull2str(char *s, unsigned long long v) {
*/
sds sdsfromlonglong(long long value) {
char buf[SDS_LLSTR_SIZE];
int len = sdsll2str(buf,value);
size_t len = sdsll2str(buf,value);

return sdsnewlen(buf,len);
}
Expand Down Expand Up @@ -583,7 +583,7 @@ sds sdscatprintf(sds s, const char *fmt, ...) {
*/
sds sdscatfmt(sds s, char const *fmt, ...) {
const char *f = fmt;
long i;
size_t i;
va_list ap;

va_start(ap,fmt);
Expand Down Expand Up @@ -789,7 +789,7 @@ int sdscmp(const sds s1, const sds s2) {
l2 = sdslen(s2);
minlen = (l1 < l2) ? l1 : l2;
cmp = memcmp(s1,s2,minlen);
if (cmp == 0) return l1-l2;
if (cmp == 0) return l1 < l2 ? -1 : 1;
return cmp;
}

Expand Down
17 changes: 10 additions & 7 deletions test.c
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
/* turn of windows warnings for _strcmp etc. */
#define _CRT_NONSTDC_NO_DEPRECATE

#include "fmacros.h"
#include "sockcompat.h"
#include <stdio.h>
Expand Down Expand Up @@ -173,7 +176,7 @@ static void send_client_tracking(redisContext *c, const char *str) {
freeReplyObject(reply);
}

static int disconnect(redisContext *c, int keep_fd) {
static redisFD disconnect(redisContext *c, int keep_fd) {
redisReply *reply;

/* Make sure we're on DB 9. */
Expand All @@ -186,9 +189,9 @@ static int disconnect(redisContext *c, int keep_fd) {

/* Free the context as well, but keep the fd if requested. */
if (keep_fd)
return redisFreeKeepFd(c);
return (int)redisFreeKeepFd(c);
redisFree(c);
return -1;
return REDIS_INVALID_FD;
}

static void do_ssl_handshake(redisContext *c) {
Expand Down Expand Up @@ -217,8 +220,8 @@ static redisContext *do_connect(struct config config) {
/* Create a dummy connection just to get an fd to inherit */
redisContext *dummy_ctx = redisConnectUnix(config.unix_sock.path);
if (dummy_ctx) {
int fd = disconnect(dummy_ctx, 1);
printf("Connecting to inherited fd %d\n", fd);
redisFD fd = disconnect(dummy_ctx, 1);
printf("Connecting to inherited fd %d\n", (int)fd);
c = redisConnectFd(fd);
}
} else {
Expand Down Expand Up @@ -251,7 +254,7 @@ static void do_reconnect(redisContext *c, struct config config) {

static void test_format_commands(void) {
char *cmd;
int len;
long long len;

test("Format command without interpolation: ");
len = redisFormatCommand(&cmd,"SET foo bar");
Expand Down Expand Up @@ -775,7 +778,7 @@ static void *hi_calloc_insecure(size_t nmemb, size_t size) {
(void)nmemb;
(void)size;
insecure_calloc_calls++;
return (void*)0xdeadc0de;
return (void*)(uintptr_t)0xdeadc0de;
}

static void *hi_realloc_fail(void *ptr, size_t size) {
Expand Down