From 554b56687a19300a75ec24184746b5512580c819 Mon Sep 17 00:00:00 2001 Date: Fri, 6 Sep 2019 15:47:09 +0100 Subject: [PATCH] fix strncpy call to avoid ASAN violation Ensure we're only reading to the size of the smallest buffer, since they're both on the stack and could potentially overlap. Overlapping is defined as ... undefined behavior. I've looked through all available implementations of strncpy and they still only copy from the first \0 found. We'll also never read past the end of sun_path since we _supply_ sun_path with a proper null terminator. --- memcached.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/memcached.c b/memcached.c index 5e170bf..eee69e9 100644 --- a/memcached.c +++ b/memcached.c @@ -2683,6 +2683,7 @@ static void conn_to_str(const conn *c, char *buf) { struct sockaddr *addr = (void *)&c->request_addr; int af; unsigned short port = 0; + size_t pathlen = 0; /* For listen ports and idle UDP ports, show listen address */ if (c->state == conn_listening || @@ -2724,10 +2725,27 @@ static void conn_to_str(const conn *c, char *buf) { break; case AF_UNIX: + // this strncpy call originally could piss off an address + // sanitizer; we supplied the size of the dest buf as a limiter, + // but optimized versions of strncpy could read past the end of + // *src while looking for a null terminator. Since buf and + // sun_path here are both on the stack they could even overlap, + // which is "undefined". In all OSS versions of strncpy I could + // find this has no effect; it'll still only copy until the first null + // terminator is found. Thus it's possible to get the OS to + // examine past the end of sun_path but it's unclear to me if this + // can cause any actual problem. + // + // We need a safe_strncpy util function but I'll punt on figuring + // that out for now. + pathlen = sizeof(((struct sockaddr_un *)addr)->sun_path); + if (MAXPATHLEN <= pathlen) { + pathlen = MAXPATHLEN - 1; + } strncpy(addr_text, ((struct sockaddr_un *)addr)->sun_path, - sizeof(addr_text) - 1); - addr_text[sizeof(addr_text)-1] = '\0'; + pathlen); + addr_text[pathlen] = '\0'; protoname = "unix"; break; }