[PATCH 0/3] daemon: fix network address handling bugs

[PATCH 0/3] daemon: fix network address handling bugs

[Sebastien Tardif via GitGitGadget]

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

[PATCH 1/3] daemon: fix IPv6 address corruption in lookup_hostname()

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/daemon.c b/daemon.c
index 0a7b1aae44..80fa0226d8 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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

Re: [PATCH 1/3] daemon: fix IPv6 address corruption in lookup_hostname()

[Junio C Hamano]

"Sebastien Tardif via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sebastien Tardif <sebtardif@ncf.ca>
>
> 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.
>
> Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
> ---
>  daemon.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 0a7b1aae44..80fa0226d8 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -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);
> +			}

The previous iteration used to more explicitly cast ai->ai_addr to
the target type, but the use of (void *) here is a cute way to make
the result shorter, which makes it a bit easier to read (it may take
readers a bit of practice to convince themselves that this type
conversion using (void *) as an intermediate type is perfectly fine,
though).

>  
> -			inet_ntop(AF_INET, &sin_addr->sin_addr,
> +			inet_ntop(ai->ai_family, addr,
>  				  addrbuf, sizeof(addrbuf));
>  			strbuf_addstr(&hi->ip_address, addrbuf);

[PATCH 2/3] daemon: fix IPv6 address truncation in ip2str()

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/daemon.c b/daemon.c
index 80fa0226d8..103c08d868 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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

[PATCH 3/3] daemon: guard NULL REMOTE_PORT in execute() logging

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemon.c b/daemon.c
index 103c08d868..78cca8673f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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

Re: [PATCH 0/3] daemon: fix network address handling bugs

[Junio C Hamano]

"Sebastien Tardif via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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 <pull.2299.git.git.1778291290159.gitgitgadget@gmail.com>,
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/pull.2300.git.git.1778773592.gitgitgadget@gmail.com/

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

> 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

Re: [PATCH 0/3] daemon: fix network address handling bugs

[Patrick Steinhardt]

On Fri, May 15, 2026 at 04:20:41AM +0900, Junio C Hamano wrote:
> "Sebastien Tardif via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > Fix three related issues in daemon.c's network address handling:
> 
> 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 <pull.2299.git.git.1778291290159.gitgitgadget@gmail.com>,
> 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/pull.2300.git.git.1778773592.gitgitgadget@gmail.com/
> 
> 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

[PATCH v2 0/3] daemon: fix network address handling bugs

[Sebastien Tardif via GitGitGadget]

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.
      
          Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
      

-- 
gitgitgadget

[PATCH v2 1/3] daemon: fix IPv6 address corruption in lookup_hostname()

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/daemon.c b/daemon.c
index 0a7b1aae44..80fa0226d8 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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

[PATCH v2 2/3] daemon: fix IPv6 address truncation in ip2str()

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/daemon.c b/daemon.c
index 80fa0226d8..103c08d868 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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

[PATCH v2 3/3] daemon: guard NULL REMOTE_PORT in execute() logging

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemon.c b/daemon.c
index 103c08d868..78cca8673f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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

Re: [PATCH v2 0/3] daemon: fix network address handling bugs

[Junio C Hamano]

"Sebastien Tardif via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

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.

> 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.

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 ;-).

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

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.

[PATCH v3 0/3] daemon: fix network address handling bugs

[Sebastien Tardif via GitGitGadget]

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.
     -
          Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
      
       ## daemon.c ##

-- 
gitgitgadget

[PATCH v3 1/3] daemon: fix IPv6 address corruption in lookup_hostname()

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/daemon.c b/daemon.c
index 0a7b1aae44..80fa0226d8 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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

[PATCH v3 2/3] daemon: fix IPv6 address truncation in ip2str()

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/daemon.c b/daemon.c
index 80fa0226d8..103c08d868 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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

[PATCH v3 3/3] daemon: guard NULL REMOTE_PORT in execute() logging

[Sebastien Tardif via GitGitGadget]

From: Sebastien Tardif <sebtardif@ncf.ca>

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.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
---
 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemon.c b/daemon.c
index 103c08d868..78cca8673f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -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