daemon: fix network address handling bugs

Fix three related issues in daemon.c's network address handling:

IPv6 address corruption in lookup_hostname(): getaddrinfo() is called with AF_UNSPEC hints, so it may return IPv6 results. However, the code unconditionally casts ai_addr to sockaddr_in and passes AF_INET to inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset, producing garbage IP addresses. Fixed by checking ai_family and handling both AF_INET and AF_INET6.

IPv6 address truncation in ip2str(): The sockaddr struct size (ai_addrlen) is passed as the output buffer size to inet_ntop(). For IPv6, sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6 addresses are silently truncated. Fixed by passing sizeof(ip) instead, and dropping the now-unused len parameter.

NULL pointer in execute() logging: REMOTE_PORT environment variable is used in a format string without a NULL check (only REMOTE_ADDR was checked). If REMOTE_PORT is unset, NULL is passed to printf's %s, which is undefined behavior. Fixed by using a fallback string.

Sebastien Tardif (3):
daemon: fix IPv6 address corruption in lookup_hostname()
daemon: fix IPv6 address truncation in ip2str()
daemon: guard NULL REMOTE_PORT in execute() logging

 daemon.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2300%2FSebTardif%2Ffix%2Fdaemon-ipv6-and-null-port-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2300/SebTardif/fix/daemon-ipv6-and-null-port-v1
Pull-Request: https://github.com/git/git/pull/2300

gitgitgadget

From: Sebastien Tardif <[email protected]>

getaddrinfo() is called with AF_UNSPEC hints, so it may return IPv6 results. However, the code unconditionally casts ai_addr to sockaddr_in and passes AF_INET to inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset, producing garbage IP addresses.

Fix this by checking ai_family and extracting the address pointer into a local variable before calling inet_ntop() once with the correct family. Die on unexpected address families.

---
 daemon.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index 0a7b1aae44..80fa0226d8 100644
@@ -674,9 +674,20 @@ static void lookup_hostname(struct hostinfo *hi)
 
 		gai = getaddrinfo(hi->hostname.buf, NULL, &hints, &ai);
 		if (!gai) {
-			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
+			void *addr;
+
+			if (ai->ai_family == AF_INET) {
+				struct sockaddr_in *sa = (void *)ai->ai_addr;
+				addr = &sa->sin_addr;
+			} else if (ai->ai_family == AF_INET6) {
+				struct sockaddr_in6 *sa6 = (void *)ai->ai_addr;
+				addr = &sa6->sin6_addr;
+			} else {
+				die("unexpected address family: %d",
+				    ai->ai_family);
+			}
 
-			inet_ntop(AF_INET, &sin_addr->sin_addr,
+			inet_ntop(ai->ai_family, addr,
 				  addrbuf, sizeof(addrbuf));
 			strbuf_addstr(&hi->ip_address, addrbuf);
 

gitgitgadget

From: Sebastien Tardif <[email protected]>

The sockaddr struct size (ai_addrlen) is passed as the output buffer size to inet_ntop(). For IPv6, sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6 addresses are silently truncated.

Fix this by passing sizeof(ip) instead, which is the actual size of the destination buffer. Drop the now-unused len parameter from ip2str() and update all callers.

---
 daemon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 80fa0226d8..103c08d868 100644
@@ -947,7 +947,7 @@ struct socketlist {
 	size_t alloc;
 };
 
-static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
+static const char *ip2str(int family, struct sockaddr *sin)
 {
 #ifdef NO_IPV6
 	static char ip[INET_ADDRSTRLEN];

@@ -958,11 +958,11 @@ static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
 	switch (family) {
 #ifndef NO_IPV6
 	case AF_INET6:
-		inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, len);
+		inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, sizeof(ip));
 		break;
 #endif
 	case AF_INET:
-		inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);
+		inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, sizeof(ip));
 		break;
 	default:
 		xsnprintf(ip, sizeof(ip), "<unknown>");

@@ -1019,14 +1019,14 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 		if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
 			logerror("Could not bind to %s: %s",
-				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+				 ip2str(ai->ai_family, ai->ai_addr),
 				 strerror(errno));
 			close(sockfd);
 			continue;	/* not fatal */
 		}
 		if (listen(sockfd, 5) < 0) {
 			logerror("Could not listen to %s: %s",
-				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+				 ip2str(ai->ai_family, ai->ai_addr),
 				 strerror(errno));
 			close(sockfd);
 			continue;	/* not fatal */

@@ -1080,7 +1080,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
 		logerror("Could not bind to %s: %s",
-			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+			 ip2str(AF_INET, (struct sockaddr *)&sin),
 			 strerror(errno));
 		close(sockfd);
 		return 0;

@@ -1088,7 +1088,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 	if (listen(sockfd, 5) < 0) {
 		logerror("Could not listen to %s: %s",
-			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+			 ip2str(AF_INET, (struct sockaddr *)&sin),
 			 strerror(errno));
 		close(sockfd);
 		return 0;

gitgitgadget

From: Sebastien Tardif <[email protected]>

The REMOTE_PORT environment variable is used in a format string without a NULL check, while REMOTE_ADDR is checked. If REMOTE_PORT is unset, NULL is passed to printf's %s, which is undefined behavior.

Add a fallback string for the NULL case.

---
 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 103c08d868..78cca8673f 100644
@@ -753,7 +753,7 @@ static int execute(void)
 	struct strvec env = STRVEC_INIT;
 
 	if (addr)
-		loginfo("Connection from %s:%s", addr, port);
+		loginfo("Connection from %s:%s", addr, port ? port : "?");
 
 	set_keep_alive(0);
 	alarm(init_timeout ? init_timeout : timeout);

gitgitgadget

Thanks for separating patches so that each of them addresses one specific issue.

It would have been better if you sent this series as [PATCH v2] as a reply to <[email protected]>, which is the previous round. That way, the mailing list archive will keep the related discussions together on the same page. If we visit the page for the cover letter I am responding to,

https://lore.kernel.org/git/[email protected]/

nobody can see that there was a previous iteration so those who looked at the earlier effort cannot refer back to it and compare.

True. Other than that though I'm happy with this iteration. Thanks!

Patrick

Fix three related issues in daemon.c's network address handling:

IPv6 address corruption in lookup_hostname(): getaddrinfo() is called with AF_UNSPEC hints, so it may return IPv6 results. However, the code unconditionally casts ai_addr to sockaddr_in and passes AF_INET to inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset, producing garbage IP addresses. Fixed by checking ai_family and handling both AF_INET and AF_INET6.

IPv6 address truncation in ip2str(): The sockaddr struct size (ai_addrlen) is passed as the output buffer size to inet_ntop(). For IPv6, sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6 addresses are silently truncated. Fixed by passing sizeof(ip) instead, and dropping the now-unused len parameter.

NULL pointer in execute() logging: REMOTE_PORT environment variable is used in a format string without a NULL check (only REMOTE_ADDR was checked). If REMOTE_PORT is unset, NULL is passed to printf's %s, which is undefined behavior. Fixed by using a fallback string.

Changes since v1:

* Split the single patch into three separate commits, one per fix, per Patrick's review.
* Deduplicated the address family handling in lookup_hostname(): instead of duplicating the inet_ntop() call for each family, the address pointer is extracted into a local void *addr variable first, then inet_ntop() is called once, per Patrick's suggestion.
* The (void *) intermediate cast on ai_addr is used intentionally: C guarantees any object pointer round-trips safely through void *, and it keeps the per-family blocks shorter than spelling out the full struct casts.
* For the REMOTE_PORT NULL guard: both REMOTE_ADDR and REMOTE_PORT are set by the same code path in handle(), so neither should be NULL independently. The guard makes the code consistent with the existing REMOTE_ADDR check and avoids undefined behavior from printf %s with a NULL argument.
* Die on unexpected address families in lookup_hostname() rather than silently leaving addrbuf uninitialized.

Sebastien Tardif (3):
daemon: fix IPv6 address corruption in lookup_hostname()
daemon: fix IPv6 address truncation in ip2str()
daemon: guard NULL REMOTE_PORT in execute() logging

 daemon.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2300%2FSebTardif%2Ffix%2Fdaemon-ipv6-and-null-port-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2300/SebTardif/fix/daemon-ipv6-and-null-port-v2
Pull-Request: https://github.com/git/git/pull/2300

Range-diff vs v1:

 1:  b2d8143811 = 1:  b2d8143811 daemon: fix IPv6 address corruption in lookup_hostname()
 2:  5c01ec3cad = 2:  5c01ec3cad daemon: fix IPv6 address truncation in ip2str()
 3:  1b2f9d1a07 ! 3:  e312735716 daemon: guard NULL REMOTE_PORT in execute() logging

@@ Metadata
## Commit message ##
daemon: guard NULL REMOTE_PORT in execute() logging

- The REMOTE_PORT environment variable is used in a format string
- without a NULL check, while REMOTE_ADDR is checked. If REMOTE_PORT
- is unset, NULL is passed to printf's %s, which is undefined behavior.
+ REMOTE_ADDR and REMOTE_PORT are both set by the same code path in
+ handle(), so neither should be NULL independently. However, the
+ existing code checks REMOTE_ADDR before the loginfo() call but not
+ REMOTE_PORT. If REMOTE_PORT were unset, NULL would be passed to
+ printf's %s, which is undefined behavior.

- Add a fallback string for the NULL case.
+ Add a fallback string for the NULL case, matching the existing
+ REMOTE_ADDR guard for consistency.

gitgitgadget

From: Sebastien Tardif <[email protected]>

getaddrinfo() is called with AF_UNSPEC hints, so it may return IPv6 results. However, the code unconditionally casts ai_addr to sockaddr_in and passes AF_INET to inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset, producing garbage IP addresses.

Fix this by checking ai_family and extracting the address pointer into a local variable before calling inet_ntop() once with the correct family. Die on unexpected address families.

---
 daemon.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index 0a7b1aae44..80fa0226d8 100644
@@ -674,9 +674,20 @@ static void lookup_hostname(struct hostinfo *hi)
 
 		gai = getaddrinfo(hi->hostname.buf, NULL, &hints, &ai);
 		if (!gai) {
-			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
+			void *addr;
+
+			if (ai->ai_family == AF_INET) {
+				struct sockaddr_in *sa = (void *)ai->ai_addr;
+				addr = &sa->sin_addr;
+			} else if (ai->ai_family == AF_INET6) {
+				struct sockaddr_in6 *sa6 = (void *)ai->ai_addr;
+				addr = &sa6->sin6_addr;
+			} else {
+				die("unexpected address family: %d",
+				    ai->ai_family);
+			}
 
-			inet_ntop(AF_INET, &sin_addr->sin_addr,
+			inet_ntop(ai->ai_family, addr,
 				  addrbuf, sizeof(addrbuf));
 			strbuf_addstr(&hi->ip_address, addrbuf);
 

gitgitgadget

From: Sebastien Tardif <[email protected]>

The sockaddr struct size (ai_addrlen) is passed as the output buffer size to inet_ntop(). For IPv6, sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6 addresses are silently truncated.

Fix this by passing sizeof(ip) instead, which is the actual size of the destination buffer. Drop the now-unused len parameter from ip2str() and update all callers.

---
 daemon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 80fa0226d8..103c08d868 100644
@@ -947,7 +947,7 @@ struct socketlist {
 	size_t alloc;
 };
 
-static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
+static const char *ip2str(int family, struct sockaddr *sin)
 {
 #ifdef NO_IPV6
 	static char ip[INET_ADDRSTRLEN];

@@ -958,11 +958,11 @@ static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
 	switch (family) {
 #ifndef NO_IPV6
 	case AF_INET6:
-		inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, len);
+		inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, sizeof(ip));
 		break;
 #endif
 	case AF_INET:
-		inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);
+		inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, sizeof(ip));
 		break;
 	default:
 		xsnprintf(ip, sizeof(ip), "<unknown>");

@@ -1019,14 +1019,14 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 		if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
 			logerror("Could not bind to %s: %s",
-				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+				 ip2str(ai->ai_family, ai->ai_addr),
 				 strerror(errno));
 			close(sockfd);
 			continue;	/* not fatal */
 		}
 		if (listen(sockfd, 5) < 0) {
 			logerror("Could not listen to %s: %s",
-				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+				 ip2str(ai->ai_family, ai->ai_addr),
 				 strerror(errno));
 			close(sockfd);
 			continue;	/* not fatal */

@@ -1080,7 +1080,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
 		logerror("Could not bind to %s: %s",
-			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+			 ip2str(AF_INET, (struct sockaddr *)&sin),
 			 strerror(errno));
 		close(sockfd);
 		return 0;

@@ -1088,7 +1088,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 	if (listen(sockfd, 5) < 0) {
 		logerror("Could not listen to %s: %s",
-			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+			 ip2str(AF_INET, (struct sockaddr *)&sin),
 			 strerror(errno));
 		close(sockfd);
 		return 0;

gitgitgadget

From: Sebastien Tardif <[email protected]>

REMOTE_ADDR and REMOTE_PORT are both set by the same code path in handle(), so neither should be NULL independently. However, the existing code checks REMOTE_ADDR before the loginfo() call but not REMOTE_PORT. If REMOTE_PORT were unset, NULL would be passed to printf's %s, which is undefined behavior.

Add a fallback string for the NULL case, matching the existing REMOTE_ADDR guard for consistency.

---
 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 103c08d868..78cca8673f 100644
@@ -753,7 +753,7 @@ static int execute(void)
 	struct strvec env = STRVEC_INIT;
 
 	if (addr)
-		loginfo("Connection from %s:%s", addr, port);
+		loginfo("Connection from %s:%s", addr, port ? port : "?");
 
 	set_keep_alive(0);
 	alarm(init_timeout ? init_timeout : timeout);

gitgitgadget

This, and all the other items in this list, are differences between the version before v1 and v2, isn't it? It is OK to pretend that the pre-v1 version v0 didn't officially exist, but it would be helpful to see the inter-version improvements for *this* version.

Indeed, range-diff tells us that the commit log improvement is the only change since the previous iteration.

This is easier to read than the previous, but it is unclear what the change is trying to achieve. You first say if addr is set port can never be unset. So by checking addr before calling loginfo(), the code effectively is ensuring that addr and port are set.

(1) The word "However" in "However the existing code checks" does not make much sense to me (I would think "Therefore" is less confusing, but if what you first said is correct, then it is quite obvious and can be left unsaid).

(2) It is unclear why "If REMOTE_PORT were unset NULL would be ..." needs to be brought up. Yes, you are not supposed to pass NULL to printf that expects "%s" to format it. But isn't the whole point of checking that addr is not NULL because the caller knows that loginfo() accesses both, and the caller also knows that if addr is not NULL, port will never be NULL? Or is this comment about something other than loginfo() where port is used without checking neither addr or port? Then it would not make much sense to bring up "addr is checked before calling loginfo()".

IOW, the sentence structure got vastly improved than the previous round, but it made it clearer that what these sentences say is unclear ;-).

I tried to find if there is any existing case (addr ? addr : "") to match, but I didn't find any. Probably that is because it is not needed (instead the code does "if (addr) ..." to protect itself).

I think the only valid justification you could give to this change is to say that even though the current code is perfectly fine as-is (i.e. as you said, addr and port are both exported at the same time so it will never happen that addr is non NULL and port is NULL), somebody who is not so careful can break that arrangement in the future, and it is a prudent thing to double check that port is not NULL before using will future-proof this part of the code.

Thanks.

Fix three related issues in daemon.c's network address handling:

IPv6 address corruption in lookup_hostname(): getaddrinfo() is called with AF_UNSPEC hints, so it may return IPv6 results. However, the code unconditionally casts ai_addr to sockaddr_in and passes AF_INET to inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset, producing garbage IP addresses. Fixed by checking ai_family and handling both AF_INET and AF_INET6.

IPv6 address truncation in ip2str(): The sockaddr struct size (ai_addrlen) is passed as the output buffer size to inet_ntop(). For IPv6, sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6 addresses are silently truncated. Fixed by passing sizeof(ip) instead, and dropping the now-unused len parameter.

NULL pointer in execute() logging: REMOTE_PORT environment variable is used in a format string without a NULL check (only REMOTE_ADDR was checked). If REMOTE_PORT is unset, NULL is passed to printf's %s, which is undefined behavior. Fixed by using a fallback string.

Changes since v1:

* Split the single patch into three separate commits, one per fix, per Patrick's review.
* Deduplicated the address family handling in lookup_hostname(): instead of duplicating the inet_ntop() call for each family, the address pointer is extracted into a local void *addr variable first, then inet_ntop() is called once, per Patrick's suggestion.
* The (void *) intermediate cast on ai_addr is used intentionally: C guarantees any object pointer round-trips safely through void *, and it keeps the per-family blocks shorter than spelling out the full struct casts.
* For the REMOTE_PORT NULL guard: both REMOTE_ADDR and REMOTE_PORT are set by the same code path in handle(), so neither should be NULL independently. The guard makes the code consistent with the existing REMOTE_ADDR check and avoids undefined behavior from printf %s with a NULL argument.
* Die on unexpected address families in lookup_hostname() rather than silently leaving addrbuf uninitialized.

Sebastien Tardif (3):
daemon: fix IPv6 address corruption in lookup_hostname()
daemon: fix IPv6 address truncation in ip2str()
daemon: guard NULL REMOTE_PORT in execute() logging

 daemon.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2300%2FSebTardif%2Ffix%2Fdaemon-ipv6-and-null-port-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2300/SebTardif/fix/daemon-ipv6-and-null-port-v3
Pull-Request: https://github.com/git/git/pull/2300

Range-diff vs v2:

 1:  b2d8143811 = 1:  b2d8143811 daemon: fix IPv6 address corruption in lookup_hostname()
 2:  5c01ec3cad = 2:  5c01ec3cad daemon: fix IPv6 address truncation in ip2str()
 3:  e312735716 ! 3:  4e74294071 daemon: guard NULL REMOTE_PORT in execute() logging

@@ Commit message
daemon: guard NULL REMOTE_PORT in execute() logging

REMOTE_ADDR and REMOTE_PORT are both set by the same code path in
- handle(), so neither should be NULL independently. However, the
- existing code checks REMOTE_ADDR before the loginfo() call but not
- REMOTE_PORT. If REMOTE_PORT were unset, NULL would be passed to
+ handle(), so when the existing REMOTE_ADDR check passes, REMOTE_PORT
+ is guaranteed to be non-NULL. Guard REMOTE_PORT as well so that a
+ future change that breaks this invariant does not pass NULL to
printf's %s, which is undefined behavior.

- Add a fallback string for the NULL case, matching the existing
- REMOTE_ADDR guard for consistency.
-

## daemon.c ##

gitgitgadget

From: Sebastien Tardif <[email protected]>

getaddrinfo() is called with AF_UNSPEC hints, so it may return IPv6 results. However, the code unconditionally casts ai_addr to sockaddr_in and passes AF_INET to inet_ntop(). On IPv6-only hosts, this reads from the wrong struct offset, producing garbage IP addresses.

Fix this by checking ai_family and extracting the address pointer into a local variable before calling inet_ntop() once with the correct family. Die on unexpected address families.

---
 daemon.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index 0a7b1aae44..80fa0226d8 100644
@@ -674,9 +674,20 @@ static void lookup_hostname(struct hostinfo *hi)
 
 		gai = getaddrinfo(hi->hostname.buf, NULL, &hints, &ai);
 		if (!gai) {
-			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
+			void *addr;
+
+			if (ai->ai_family == AF_INET) {
+				struct sockaddr_in *sa = (void *)ai->ai_addr;
+				addr = &sa->sin_addr;
+			} else if (ai->ai_family == AF_INET6) {
+				struct sockaddr_in6 *sa6 = (void *)ai->ai_addr;
+				addr = &sa6->sin6_addr;
+			} else {
+				die("unexpected address family: %d",
+				    ai->ai_family);
+			}
 
-			inet_ntop(AF_INET, &sin_addr->sin_addr,
+			inet_ntop(ai->ai_family, addr,
 				  addrbuf, sizeof(addrbuf));
 			strbuf_addstr(&hi->ip_address, addrbuf);
 

gitgitgadget

From: Sebastien Tardif <[email protected]>

The sockaddr struct size (ai_addrlen) is passed as the output buffer size to inet_ntop(). For IPv6, sizeof(sockaddr_in6) is 28 bytes but INET6_ADDRSTRLEN is 46, so long IPv6 addresses are silently truncated.

Fix this by passing sizeof(ip) instead, which is the actual size of the destination buffer. Drop the now-unused len parameter from ip2str() and update all callers.

---
 daemon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 80fa0226d8..103c08d868 100644
@@ -947,7 +947,7 @@ struct socketlist {
 	size_t alloc;
 };
 
-static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
+static const char *ip2str(int family, struct sockaddr *sin)
 {
 #ifdef NO_IPV6
 	static char ip[INET_ADDRSTRLEN];

@@ -958,11 +958,11 @@ static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
 	switch (family) {
 #ifndef NO_IPV6
 	case AF_INET6:
-		inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, len);
+		inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, sizeof(ip));
 		break;
 #endif
 	case AF_INET:
-		inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);
+		inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, sizeof(ip));
 		break;
 	default:
 		xsnprintf(ip, sizeof(ip), "<unknown>");

@@ -1019,14 +1019,14 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 		if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
 			logerror("Could not bind to %s: %s",
-				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+				 ip2str(ai->ai_family, ai->ai_addr),
 				 strerror(errno));
 			close(sockfd);
 			continue;	/* not fatal */
 		}
 		if (listen(sockfd, 5) < 0) {
 			logerror("Could not listen to %s: %s",
-				 ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+				 ip2str(ai->ai_family, ai->ai_addr),
 				 strerror(errno));
 			close(sockfd);
 			continue;	/* not fatal */

@@ -1080,7 +1080,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 	if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
 		logerror("Could not bind to %s: %s",
-			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+			 ip2str(AF_INET, (struct sockaddr *)&sin),
 			 strerror(errno));
 		close(sockfd);
 		return 0;

@@ -1088,7 +1088,7 @@ static int setup_named_sock(char *listen_addr, int listen_port, struct socketlis
 
 	if (listen(sockfd, 5) < 0) {
 		logerror("Could not listen to %s: %s",
-			 ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+			 ip2str(AF_INET, (struct sockaddr *)&sin),
 			 strerror(errno));
 		close(sockfd);
 		return 0;

gitgitgadget

From: Sebastien Tardif <[email protected]>

REMOTE_ADDR and REMOTE_PORT are both set by the same code path in handle(), so when the existing REMOTE_ADDR check passes, REMOTE_PORT is guaranteed to be non-NULL. Guard REMOTE_PORT as well so that a future change that breaks this invariant does not pass NULL to printf's %s, which is undefined behavior.

---
 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 103c08d868..78cca8673f 100644
@@ -753,7 +753,7 @@ static int execute(void)
 	struct strvec env = STRVEC_INIT;
 
 	if (addr)
-		loginfo("Connection from %s:%s", addr, port);
+		loginfo("Connection from %s:%s", addr, port ? port : "?");
 
 	set_keep_alive(0);
 	alarm(init_timeout ? init_timeout : timeout);

gitgitgadget