[BUG][MinGW64] Error setting socket timeout

[dellorogiulio]

Hi,
I'm recently working with the redis-plus-plus repo which wraps hiredis and I found a very annoying bug: setting a socket timeout of 10ms, it becomes 10s. I'm running MinGW under Windows.
Analyzing the problem with @sewenew, which is the creator of redis-plus-plus, we could have found the bug.
Let me summarize the problem:
net.c

316    int redisContextSetTimeout(redisContext *c, const struct timeval tv) {
317        const void *to_ptr = &tv;
318        size_t to_sz = sizeof(tv);
319    #ifdef _WIN32
320        DWORD timeout_msec = tv.tv_sec * 1000 + tv.tv_usec / 1000;
321        to_ptr = &timeout_msec;
322       to_sz = sizeof(timeout_msec);
323    #endif
324        if (setsockopt(c->fd,SOL_SOCKET,SO_RCVTIMEO,to_ptr,to_sz) == -1) {
325            __redisSetErrorFromErrno(c,REDIS_ERR_IO,"setsockopt(SO_RCVTIMEO)");
326            return REDIS_ERR;
327        }
328        if (setsockopt(c->fd,SOL_SOCKET,SO_SNDTIMEO,to_ptr,to_sz) == -1) {
329            __redisSetErrorFromErrno(c,REDIS_ERR_IO,"setsockopt(SO_SNDTIMEO)");
330            return REDIS_ERR;
331        }
332       return REDIS_OK;
333    }

Line 320 a DWORD timeout_msec is correctly computed from the timeval tv
Line 321 to_ptr is assign to the address of timeout_msec (to_ptr is di facto a DWORD*)
Line 324 to_ptr is pass to setsockopt which is win32_setsockoption

sockcompat.c

212    int win32_setsockopt(SOCKET sockfd, int level, int optname, const void *optval, socklen_t optlen) {
213        int ret = 0;
214        if ((level == SOL_SOCKET) && ((optname == SO_RCVTIMEO) || (optname == SO_SNDTIMEO))) {
215            struct timeval *tv = optval;
216            DWORD timeout = tv->tv_sec * 1000 + tv->tv_usec / 1000;
217            ret = setsockopt(sockfd, level, optname, (const char*)&timeout, sizeof(DWORD));
218        } else {
219            ret = setsockopt(sockfd, level, optname, (const char*)optval, optlen);
220        }
221        _updateErrno(ret != SOCKET_ERROR);
222        return ret != SOCKET_ERROR ? ret : -1;
223    }

Line 215 casts optval (which is a DWORD*) to timeval* and this obviously causes the error.

I don't understand why the #ifdef _WIN32 was introduced at line 320, In fact, commenting out line 320, 321, 322, 323 all seems work as expected! Now the timeout is called after the correct amount of milliseconds!
Thanks in advance

[michael-grunder]

From a quick glance it looks like a mixup between two separate commits/PRs.

When ab1762c was committed win32_setsockopt did not have struct timeval -> DWORD conversion logic but that was added later in d8f814d, creating the conflict.

I can put together a patch but could use help testing the chance since I do not own a windows machine. It might also be a good idea to add a specific MinGW timeout test to protect against a regression.

[dellorogiulio]

Hi,
I'm obviously available to run some MinGW test! When a patch is ready let me know so I can test it.
Thanks!

[michael-grunder]

Hi @dellorogiulio could you try the branch I'm working on over at my fork of the repo.

To hopefully prevent this from happening again I spent some time getting the unit tests to compile and run in Windows.

Let me know if it's working for you!

[dellorogiulio]

Hi @michael-grunder
I compile and test the testing branch: all seems good!

#01 Format command without interpolation: PASSED
#02 Format command with %s string interpolation: PASSED
#03 Format command with %s and an empty string: PASSED
#04 Format command with an empty string in between proper interpolations: PASSED
#05 Format command with %b string interpolation: PASSED
#06 Format command with %b and an empty string: PASSED
#07 Format command with literal %: PASSED
#08 Format command with printf-delegation (int): PASSED
#09 Format command with printf-delegation (char): PASSED
#10 Format command with printf-delegation (short): PASSED
#11 Format command with printf-delegation (long): PASSED
#12 Format command with printf-delegation (long long): PASSED
#13 Format command with printf-delegation (unsigned int): PASSED
#14 Format command with printf-delegation (unsigned char): PASSED
#15 Format command with printf-delegation (unsigned short): PASSED
#16 Format command with printf-delegation (unsigned long): PASSED
#17 Format command with printf-delegation (unsigned long long): PASSED
#18 Format command with printf-delegation (float): PASSED
#19 Format command with printf-delegation (double): PASSED
#20 Format command with invalid printf format: PASSED
#21 Format command by passing argc/argv without lengths: PASSED
#22 Format command by passing argc/argv with lengths: PASSED
#23 Format command into sds by passing argc/argv without lengths: PASSED
#24 Format command into sds by passing argc/argv with lengths: PASSED
#25 Error handling in reply parser: PASSED
#26 Memory cleanup in reply parser: PASSED
#27 Set error on nested multi bulks with depth > 7: PASSED
#28 Correctly parses LLONG_MAX: PASSED
#29 Set error when > LLONG_MAX: PASSED
#30 Correctly parses LLONG_MIN: PASSED
#31 Set error when < LLONG_MIN: PASSED
#32 Set error when array < -1: PASSED
#33 Set error when bulk < -1: PASSED
#34 Works with NULL functions for reply: PASSED
#35 Works when a single newline (\r\n) covers two calls to feed: PASSED
#36 Don't reset state after protocol error: PASSED
#37 Don't do empty allocation for empty multi bulk: PASSED
#38 Returns error when host cannot be resolved: PASSED
#39 Don't fail when redisFree is passed a NULL value: PASSED
#40 Don't fail when freeReplyObject is passed a NULL value: PASSED

Testing against TCP connection (127.0.0.1:6379):
#41 Is able to deliver commands: PASSED
#42 Is a able to send commands verbatim: PASSED
#43 %s String interpolation works: PASSED
#44 %b String interpolation works: PASSED
#45 Binary reply length is correct: PASSED
#46 Can parse nil replies: PASSED
#47 Can parse integer replies: PASSED
#48 Can parse multi bulk replies: PASSED
#49 Can handle nested multi bulk replies: PASSED
#50 Can pass NULL to redisGetReply: PASSED
#51 Successfully completes a command when the timeout is not exceeded: PASSED
#52 Does not return a reply when the command times out: PASSED
#53 Reconnect properly reconnects after a timeout: PASSED
#54 Reconnect properly uses owned parameters: PASSED
#55 Returns I/O error when the connection is lost: PASSED
#56 Returns I/O error on socket timeout: PASSED
#57 Set error when an invalid timeout usec value is given to redisConnectWithTimeout: PASSED
#58 Set error when an invalid timeout sec value is given to redisConnectWithTimeout: PASSED
#59 Append format command: PASSED
#60 Throughput:
        (1000x PING: 0.094s)
        (1000x LRANGE with 500 elements: 0.313s)
        (1000x INCRBY: 0.091s)
        (10000x PING (pipelined): 0.031s)
        (10000x LRANGE with 500 elements (pipelined): 2.984s)
        (10000x INCRBY (pipelined): 0.031s)

Testing against Unix socket connection (/tmp/redis.sock): SKIPPED

Testing against inherited fd (/tmp/redis.sock): SKIPPED
ALL TESTS PASSED

The wrong timeout is now fixed (I already test it by myself editing net.c as you did in testing branch)

I could imagine that you will merge with master soon, so I'm going to close this issue.

Thanks!