`redisConnectUnix`: `strncpy` of `sun_path` omits the NUL when the path length is `sizeof(sun_path)-1` or more

[yayong3]

Summary

redisContextConnectUnix() copies the socket path with:

strncpy(sa->sun_path, path, sizeof(sa->sun_path) - 1);

sa comes from hi_malloc (not calloc). strncpy of n bytes does not write a terminator when strlen(path) >= n.

On Linux sizeof(sun_path) is 108, so a path of length 107 is copied as 107 bytes with no '\0', and sun_path[107] is whatever hi_malloc returned. connect() is then called with addrlen = sizeof(struct sockaddr_un), so the kernel reads the entire sun_path array.

Short paths (/tmp/redis.sock) are unaffected. There is no REDIS_ERR / errstr for “path too long”; the call either hits the wrong address or fails in a way that depends on heap contents.

Present on 1.5.0-dev (29ea279), net.c. Public API: redisConnectUnix, redisConnectUnixWithTimeout, redisAsyncConnectUnix.

Code

/* net.c — redisContextConnectUnix */

sa = (struct sockaddr_un *)(c->saddr = hi_malloc(sizeof(struct sockaddr_un)));
if (sa == NULL)
    goto oom;

c->addrlen = sizeof(struct sockaddr_un);
sa->sun_family = AF_UNIX;
strncpy(sa->sun_path, path, sizeof(sa->sun_path) - 1);  /* :688 */
if (connect(c->fd, (struct sockaddr *)sa, sizeof(*sa)) == -1) {

hi_malloc does not zero the object. Only sun_family is assigned before the copy. If strlen(path) >= sizeof(sun_path) - 1, sun_path contains no NUL.

POSIX connect for AF_UNIX uses addrlen to bound sun_path. Passing sizeof(*sa) includes the full sun_path array, so a missing terminator is visible to the kernel.

The same buffer is left in the public redisContext.saddr field after the call.

Reproduce

No Redis server. Fill malloc with 0xAB so a missing terminator is visible, then call redisConnectUnix with a path of length sizeof(sun_path) - 1 and inspect c->saddr (the object passed to connect(2)).

make USE_WERROR=0 static
cc -O0 -g -std=c99 -I. -o poc poc.c libhiredis.a
./poc

#define _POSIX_C_SOURCE 200809L
#include "hiredis.h"
#include "alloc.h"

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/un.h>

static void *fill_malloc(size_t n)
{
    void *p = malloc(n);
    if (p)
        memset(p, 0xAB, n);
    return p;
}

int main(void)
{
    hiredisAllocFuncs ha = {
        .mallocFn = fill_malloc,
        .callocFn = calloc,
        .reallocFn = realloc,
        .strdupFn = strdup,
        .freeFn = free,
    };
    size_t cap = sizeof((struct sockaddr_un){0}.sun_path);
    char *path = malloc(cap);
    size_t i;
    redisContext *c;
    const struct sockaddr_un *un;
    const char *nul;
    unsigned last;

    hiredisSetAllocators(&ha);
    for (i = 0; i < cap - 1; i++)
        path[i] = 'a';
    path[cap - 1] = '\0';

    fprintf(stderr, "path_len=%zu sun_path_cap=%zu\n", strlen(path), cap);
    c = redisConnectUnix(path);
    if (c == NULL || c->saddr == NULL)
        return 1;

    un = (const struct sockaddr_un *)c->saddr;
    nul = memchr(un->sun_path, 0, sizeof un->sun_path);
    last = (unsigned char)un->sun_path[sizeof un->sun_path - 1];
    fprintf(stderr, "err=%d errstr=%s addrlen=%zu last=0x%02x nul=%s\n",
            c->err, c->errstr[0] ? c->errstr : "(none)",
            c->addrlen, last, nul ? "yes" : "NO");

    redisFree(c);
    free(path);
    return nul ? 1 : 0;
}

hiredisSetAllocators is a public API (alloc.h). It only makes the uninitialized tail deterministic (0xAB). The missing NUL is from strncpy, not from the custom allocator.

Observed

Linux x86_64, sizeof(sun_path) == 108, hiredis 29ea279:

path_len=107 sun_path_cap=108
err=1 errstr=No such file or directory addrlen=110 last=0xab nul=NO

• path_len == cap - 1 is the boundary at which strncpy(..., cap - 1) does not write a terminator.
• last=0xab is the untouched tail of the hi_malloc buffer, not '\0'.
• addrlen=110 is sizeof(struct sockaddr_un) (sun_family + 108-byte sun_path).
• errstr=No such file or directory is the kernel’s verdict on that unterminated name, not a hiredis “path too long” error.

Intercepting connect(2) shows the same bytes: cap=108 addrlen=110 last=0xab nul=NO.

Suggested fix

Zero the sockaddr_un, reject overlong paths with a clear error, and always terminate:

--- a/net.c
+++ b/net.c
@@ -684,9 +684,16 @@ int redisContextConnectUnix(...)
         goto oom;

     c->addrlen = sizeof(struct sockaddr_un);
+    memset(sa, 0, sizeof(*sa));
     sa->sun_family = AF_UNIX;
-    strncpy(sa->sun_path, path, sizeof(sa->sun_path) - 1);
+    if (strlen(path) >= sizeof(sa->sun_path)) {
+        __redisSetError(c, REDIS_ERR_OTHER, "Unix socket path too long");
+        return REDIS_ERR;
+    }
+    memcpy(sa->sun_path, path, strlen(path) + 1);
     if (connect(c->fd, (struct sockaddr*)sa, sizeof(*sa)) == -1) {

memset alone would make a truncated 107-byte name accidentally NUL-terminated at byte 107 and silently connect to the wrong path. An explicit length check is preferable.

Environment

• redis/hiredis 29ea279 (1.5.0-dev)
• Linux x86_64, sizeof(sun_path) == 108, sizeof(struct sockaddr_un) == 110