diff --git a/include/tscore/ink_inet.h b/include/tscore/ink_inet.h index 18319b13535..8f463d0c8bb 100644 --- a/include/tscore/ink_inet.h +++ b/include/tscore/ink_inet.h @@ -1610,9 +1610,23 @@ struct UnAddr { UnAddr() { _path[0] = 0; } - UnAddr(self const &addr) { strncpy(_path, addr._path, TS_UNIX_SIZE); } - explicit UnAddr(const char *path) { strncpy(_path, path, TS_UNIX_SIZE - 1); } - explicit UnAddr(const std::string &path) { strncpy(_path, path.c_str(), TS_UNIX_SIZE); } + // strncpy writes no terminator when it truncates, so every path that fills _path + // terminates it explicitly. + UnAddr(self const &addr) + { + strncpy(_path, addr._path, TS_UNIX_SIZE - 1); + _path[TS_UNIX_SIZE - 1] = '\0'; + } + explicit UnAddr(const char *path) + { + strncpy(_path, path, TS_UNIX_SIZE - 1); + _path[TS_UNIX_SIZE - 1] = '\0'; + } + explicit UnAddr(const std::string &path) + { + strncpy(_path, path.c_str(), TS_UNIX_SIZE - 1); + _path[TS_UNIX_SIZE - 1] = '\0'; + } explicit UnAddr(sockaddr const *addr) { this->assign(addr); } explicit UnAddr(sockaddr_un const *addr) { this->assign(ats_ip_sa_cast(addr)); } @@ -1639,7 +1653,8 @@ struct UnAddr { operator=(self const &addr) { if (this != &addr) { - strncpy(_path, addr._path, TS_UNIX_SIZE); + strncpy(_path, addr._path, TS_UNIX_SIZE - 1); + _path[TS_UNIX_SIZE - 1] = '\0'; } return *this; } @@ -1651,7 +1666,9 @@ inline UnAddr & UnAddr::assign(sockaddr const *addr) { if (addr) { - strncpy(_path, ats_unix_cast(addr)->sun_path, TS_UNIX_SIZE); + // A kernel sockaddr_un may carry a full, unterminated sun_path. + strncpy(_path, ats_unix_cast(addr)->sun_path, TS_UNIX_SIZE - 1); + _path[TS_UNIX_SIZE - 1] = '\0'; } return *this; } diff --git a/src/records/RecHttp.cc b/src/records/RecHttp.cc index 57cb822c994..7bc522ba60d 100644 --- a/src/records/RecHttp.cc +++ b/src/records/RecHttp.cc @@ -368,10 +368,16 @@ HttpProxyPort::processOptions(const char *opts) for (auto item : values) { if (item[0] == '/') { - m_family = AF_UNIX; - m_unix_path = UnAddr(item); - af_set_p = true; - zret = true; + if (size_t len = strlen(item); len >= TS_UNIX_SIZE) { + // A longer path would be silently truncated and the listener bound to the wrong + // filesystem path. + Warning("Unix path '%s' in port configuration '%s' is too long (%zu bytes, max %zu)", item, opts, len, TS_UNIX_SIZE - 1); + } else { + m_family = AF_UNIX; + m_unix_path = UnAddr(item); + af_set_p = true; + zret = true; + } } else if (isdigit(item[0])) { // leading digit -> port value char *ptr; int port = strtoul(item, &ptr, 10); diff --git a/src/records/unit_tests/test_RecHttp.cc b/src/records/unit_tests/test_RecHttp.cc index 9f0190d51f7..9f9f97523c1 100644 --- a/src/records/unit_tests/test_RecHttp.cc +++ b/src/records/unit_tests/test_RecHttp.cc @@ -97,6 +97,35 @@ TEST_CASE("RecHttp", "[librecords][RecHttp]") REQUIRE(view.find(":ssl") != TextView::npos); REQUIRE(view.find(":proto") == TextView::npos); // it's default, should not have this. } + + SECTION("unix-path") + { + HttpProxyPort::loadValue(ports, "/run/ats/uds.socket"); + REQUIRE(ports.size() == 1); + REQUIRE(ports[0].m_family == AF_UNIX); + REQUIRE(std::string_view(ports[0].m_unix_path._path) == "/run/ats/uds.socket"); + } + + SECTION("unix-path-max") + { + // Exactly TS_UNIX_SIZE - 1 bytes: the longest path that fits sun_path with its terminator. + std::string path = "/" + std::string(TS_UNIX_SIZE - 2, 'x'); + HttpProxyPort::loadValue(ports, path.c_str()); + REQUIRE(ports.size() == 1); + REQUIRE(ports[0].m_family == AF_UNIX); + REQUIRE(std::string_view(ports[0].m_unix_path._path) == path); + } + + SECTION("unix-path-too-long") + { + // One byte past what sun_path can hold. + std::string path = "/" + std::string(TS_UNIX_SIZE - 1, 'x'); + HttpProxyPort::loadValue(ports, path.c_str()); + REQUIRE(ports.size() == 0); + // The reject itself, then loadValue's "No valid definition" for the dropped descriptor. + REQUIRE(cdiag->messages.size() == 2); + REQUIRE(cdiag->messages[0].find("too long") != std::string::npos); + } } struct ConvertAlpnToWireFormatTestCase { diff --git a/src/tscore/unit_tests/test_ink_inet.cc b/src/tscore/unit_tests/test_ink_inet.cc index b16234041aa..034012e9467 100644 --- a/src/tscore/unit_tests/test_ink_inet.cc +++ b/src/tscore/unit_tests/test_ink_inet.cc @@ -280,4 +280,27 @@ TEST_CASE("ink_inet_unix", "[libts][inet][unix]") #if HAVE_STRUCT_SOCKADDR_UN_SUN_LEN REQUIRE(ep.sun.sun_len == SUN_LEN(&ep.sun)); #endif + + // An over-long path must still yield a null terminated _path. + std::string long_path(TS_UNIX_SIZE + 10, 'y'); + UnAddr from_cstr(long_path.c_str()); + REQUIRE(from_cstr._path[TS_UNIX_SIZE - 1] == '\0'); + REQUIRE(strlen(from_cstr._path) == TS_UNIX_SIZE - 1); + UnAddr from_string(long_path); + REQUIRE(from_string._path[TS_UNIX_SIZE - 1] == '\0'); + REQUIRE(strlen(from_string._path) == TS_UNIX_SIZE - 1); + + // A kernel sockaddr_un may carry a full, unterminated sun_path; assign() and the copy + // paths must still yield a terminated _path. + IpEndpoint raw; + raw.sa.sa_family = AF_UNIX; + memset(raw.sun.sun_path, 'z', TS_UNIX_SIZE); // deliberately no terminator + UnAddr from_sockaddr(&raw.sa); + REQUIRE(from_sockaddr._path[TS_UNIX_SIZE - 1] == '\0'); + REQUIRE(strlen(from_sockaddr._path) == TS_UNIX_SIZE - 1); + UnAddr copied(from_sockaddr); + REQUIRE(copied._path[TS_UNIX_SIZE - 1] == '\0'); + UnAddr assigned; + assigned = from_sockaddr; + REQUIRE(assigned._path[TS_UNIX_SIZE - 1] == '\0'); }