daemon: use strbuf for hostname info

PATCH In review René Scharfe 2 participants 9 messages Jun 15, 2016

Share & tools

René Scharfe

author

2016-06-15 23:04 UTC #0 lore raw

PATCH 1/1 daemon: use strbuf for hostname info

Convert hostname, canon_hostname, ip_address and tcp_port to strbuf. This allows to get rid of the helpers strbuf_addstr_or_null() and STRARG because a strbuf always represents a valid (initially empty) string. sanitize_client() becomes unused and is removed as well.

Signed-off-by Rene Scharfe <[email protected]>

---
 daemon.c | 98 +++++++++++++++++++++++++++-------------------------------------
 1 file changed, 41 insertions(+), 57 deletions(-)

daemon.c +41 ~~-57~~

index c3edd96..c04315e 100644
@@ -56,10 +56,10 @@ static const char *user_path;
 static unsigned int timeout;
 static unsigned int init_timeout;
 
-static char *hostname;
-static char *canon_hostname;
-static char *ip_address;
-static char *tcp_port;
+static struct strbuf hostname = STRBUF_INIT;
+static struct strbuf canon_hostname = STRBUF_INIT;
+static struct strbuf ip_address = STRBUF_INIT;
+static struct strbuf tcp_port = STRBUF_INIT;
 
 static int hostname_lookup_done;

@@ -68,13 +68,13 @@ static void lookup_hostname(void);
 static const char *get_canon_hostname(void)
 {
 	lookup_hostname();
-	return canon_hostname;
+	return canon_hostname.buf;
 }
 
 static const char *get_ip_address(void)
 {
 	lookup_hostname();
-	return ip_address;
+	return ip_address.buf;
 }
 
 static void logreport(int priority, const char *err, va_list params)

@@ -122,12 +122,6 @@ static void NORETURN daemon_die(const char *err, va_list params)
 	exit(1);
 }
 
-static void strbuf_addstr_or_null(struct strbuf *sb, const char *s)
-{
-	if (s)
-		strbuf_addstr(sb, s);
-}
-
 struct expand_path_context {
 	const char *directory;
 };

@@ -138,22 +132,22 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
 
 	switch (placeholder[0]) {
 	case 'H':
-		strbuf_addstr_or_null(sb, hostname);
+		strbuf_addbuf(sb, &hostname);
 		return 1;
 	case 'C':
 		if (placeholder[1] == 'H') {
-			strbuf_addstr_or_null(sb, get_canon_hostname());
+			strbuf_addstr(sb, get_canon_hostname());
 			return 2;
 		}
 		break;
 	case 'I':
 		if (placeholder[1] == 'P') {
-			strbuf_addstr_or_null(sb, get_ip_address());
+			strbuf_addstr(sb, get_ip_address());
 			return 2;
 		}
 		break;
 	case 'P':
-		strbuf_addstr_or_null(sb, tcp_port);
+		strbuf_addbuf(sb, &tcp_port);
 		return 1;
 	case 'D':
 		strbuf_addstr(sb, context->directory);

@@ -301,16 +295,14 @@ static int run_access_hook(struct daemon_service *service, const char *dir, cons
 	char *eol;
 	int seen_errors = 0;
 
-#define STRARG(x) ((x) ? (x) : "")
 	*arg++ = access_hook;
 	*arg++ = service->name;
 	*arg++ = path;
-	*arg++ = STRARG(hostname);
-	*arg++ = STRARG(get_canon_hostname());
-	*arg++ = STRARG(get_ip_address());
-	*arg++ = STRARG(tcp_port);
+	*arg++ = hostname.buf;
+	*arg++ = get_canon_hostname();
+	*arg++ = get_ip_address();
+	*arg++ = tcp_port.buf;
 	*arg = NULL;
-#undef STRARG
 
 	child.use_shell = 1;
 	child.argv = argv;

@@ -556,23 +548,14 @@ static void sanitize_client_strbuf(struct strbuf *out, const char *in)
 		strbuf_setlen(out, out->len - 1);
 }
 
-static char *sanitize_client(const char *in)
-{
-	struct strbuf out = STRBUF_INIT;
-	sanitize_client_strbuf(&out, in);
-	return strbuf_detach(&out, NULL);
-}
-
 /*
- * Like sanitize_client, but we also perform any canonicalization
+ * Like sanitize_client_strbuf, but we also perform any canonicalization
  * to make life easier on the admin.
  */
-static char *canonicalize_client(const char *in)
+static void canonicalize_client_strbuf(struct strbuf *out, const char *in)
 {
-	struct strbuf out = STRBUF_INIT;
-	sanitize_client_strbuf(&out, in);
-	strbuf_tolower(&out);
-	return strbuf_detach(&out, NULL);
+	sanitize_client_strbuf(out, in);
+	strbuf_tolower(out);
 }
 
 /*

@@ -595,11 +578,11 @@ static void parse_host_arg(char *extra_args, int buflen)
 				char *port;
 				parse_host_and_port(val, &host, &port);
 				if (port) {
-					free(tcp_port);
-					tcp_port = sanitize_client(port);
+					strbuf_reset(&tcp_port);
+					sanitize_client_strbuf(&tcp_port, port);
 				}
-				free(hostname);
-				hostname = canonicalize_client(host);
+				strbuf_reset(&hostname);
+				canonicalize_client_strbuf(&hostname, host);
 				hostname_lookup_done = 0;
 			}

@@ -616,7 +599,7 @@ static void parse_host_arg(char *extra_args, int buflen)
  */
 static void lookup_hostname(void)
 {
-	if (!hostname_lookup_done && hostname) {
+	if (!hostname_lookup_done && hostname.len) {
 #ifndef NO_IPV6
 		struct addrinfo hints;
 		struct addrinfo *ai;

@@ -626,19 +609,21 @@ static void lookup_hostname(void)
 		memset(&hints, 0, sizeof(hints));
 		hints.ai_flags = AI_CANONNAME;
 
-		gai = getaddrinfo(hostname, NULL, &hints, &ai);
+		gai = getaddrinfo(hostname.buf, NULL, &hints, &ai);
 		if (!gai) {
 			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
 
 			inet_ntop(AF_INET, &sin_addr->sin_addr,
 				  addrbuf, sizeof(addrbuf));
-			free(ip_address);
-			ip_address = xstrdup(addrbuf);
+			strbuf_reset(&ip_address);
+			strbuf_addstr(&ip_address, addrbuf);
 
-			free(canon_hostname);
-			canon_hostname = ai->ai_canonname ?
-				sanitize_client(ai->ai_canonname) :
-				xstrdup(ip_address);
+			strbuf_reset(&canon_hostname);
+			if (ai->ai_canonname)
+				sanitize_client_strbuf(&canon_hostname,
+						       ai->ai_canonname);
+			else
+				strbuf_addbuf(&canon_hostname, &ip_address);
 
 			freeaddrinfo(ai);
 		}

@@ -648,7 +633,7 @@ static void lookup_hostname(void)
 		char **ap;
 		static char addrbuf[HOST_NAME_MAX + 1];
 
-		hent = gethostbyname(hostname);
+		hent = gethostbyname(hostname.buf);
 		if (hent) {
 			ap = hent->h_addr_list;
 			memset(&sa, 0, sizeof sa);

@@ -659,10 +644,10 @@ static void lookup_hostname(void)
 			inet_ntop(hent->h_addrtype, &sa.sin_addr,
 				  addrbuf, sizeof(addrbuf));
 
-			free(canon_hostname);
-			canon_hostname = sanitize_client(hent->h_name);
-			free(ip_address);
-			ip_address = xstrdup(addrbuf);
+			strbuf_reset(&canon_hostname);
+			sanitize_client_strbuf(&canon_hostname, hent->h_name);
+			strbuf_reset(&ip_address);
+			strbuf_addstr(&ip_address, addrbuf);
 		}
 #endif
 		hostname_lookup_done = 1;

@@ -693,11 +678,10 @@ static int execute(void)
 		pktlen--;
 	}
 
-	free(hostname);
-	free(canon_hostname);
-	free(ip_address);
-	free(tcp_port);
-	hostname = canon_hostname = ip_address = tcp_port = NULL;
+	strbuf_reset(&hostname);
+	strbuf_reset(&canon_hostname);
+	strbuf_reset(&ip_address);
+	strbuf_reset(&tcp_port);
 
 	if (len != pktlen)
 		parse_host_arg(line + len + 1, pktlen - len - 1);

Signature · 1 line

2.3.1

Jeff King

2016-06-15 23:04 UTC #1 lore raw

Quoted 4 lines · René Scharfe

Convert hostname, canon_hostname, ip_address and tcp_port to strbuf. This allows to get rid of the helpers strbuf_addstr_or_null() and STRARG because a strbuf always represents a valid (initially empty) string. sanitize_client() becomes unused and is removed as well.

Makes sense. I had a feeling that we might have cared about NULL versus the empty string somewhere, but I did not see it in the patch below, so I think it is fine.

daemon.c static void sanitize_client_strbuf(struct strbuf *out, const char *in) · line 564

Quoted 6 lines

daemon.c +0 -6

-static char *sanitize_client(const char *in)
-{
-	struct strbuf out = STRBUF_INIT;
-	sanitize_client_strbuf(&out, in);
-	return strbuf_detach(&out, NULL);
-}

Not a big deal, but do we want to rename sanitize_client_strbuf to sanitize_client? It only had the unwieldy name to distinguish it from this one.

daemon.c static void parse_host_arg(char *extra_args, int buflen) · line 582

Quoted 5 lines

daemon.c +2 -2

 				if (port) {
-					free(tcp_port);
-					tcp_port = sanitize_client(port);
+					strbuf_reset(&tcp_port);
+					sanitize_client_strbuf(&tcp_port, port);

The equivalent of free() is strbuf_release(). I think it is reasonable to strbuf_reset here, since we are about to write into it again anyway (though I doubt it happens much in practice, since that would imply multiple host= segments sent by the client). But later...

daemon.c static int execute(void) · line 684

Quoted 9 lines

daemon.c +4 -5

-	free(hostname);
-	free(canon_hostname);
-	free(ip_address);
-	free(tcp_port);
-	hostname = canon_hostname = ip_address = tcp_port = NULL;
+	strbuf_reset(&hostname);
+	strbuf_reset(&canon_hostname);
+	strbuf_reset(&ip_address);
+	strbuf_reset(&tcp_port);

These probably want to all be strbuf_release(). Again, I doubt it matters much because this is a forked daemon serving only a single request (so they'll get freed by the OS soon anyway), but I think freeing the memory here follows the original intent.

-Peff

René Scharfe

author

2016-06-15 23:04 UTC #2 lore raw

Am 06.03.2015 um 22:06 schrieb Jeff King:

Quoted 11 lines · René Scharfe

Quoted 5 lines
Convert hostname, canon_hostname, ip_address and tcp_port to strbuf. This allows to get rid of the helpers strbuf_addstr_or_null() and STRARG because a strbuf always represents a valid (initially empty) string. sanitize_client() becomes unused and is removed as well.
Makes sense. I had a feeling that we might have cared about NULL versus the empty string somewhere, but I did not see it in the patch below, so I think it is fine.

Quoted 10 lines

Review
Quoted 6 lines
(quoted diff) +0 -6
-static char *sanitize_client(const char *in)
-{
-	struct strbuf out = STRBUF_INIT;
-	sanitize_client_strbuf(&out, in);
-	return strbuf_detach(&out, NULL);
-}
Not a big deal, but do we want to rename sanitize_client_strbuf to sanitize_client? It only had the unwieldy name to distinguish it from this one.

A patch would look like this. The result is shorter, but no win in terms of vertical space (number of lines).

-- >8 --
Subject: daemon: drop _strbuf suffix of sanitize and canonicalize functions

Now that only the strbuf variants of the functions remain, remove the "_strbuf" part from their names.

Suggested-by Jeff King <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
 daemon.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

daemon.c +9 -9

index c04315e..0412f8c 100644
@@ -534,7 +534,7 @@ static void parse_host_and_port(char *hostport, char **host,
  * trailing and leading dots, which means that the client cannot escape
  * our base path via ".." traversal.
  */
-static void sanitize_client_strbuf(struct strbuf *out, const char *in)
+static void sanitize_client(struct strbuf *out, const char *in)
 {
 	for (; *in; in++) {
 		if (*in == '/')

@@ -549,12 +549,12 @@ static void sanitize_client_strbuf(struct strbuf *out, const char *in)
 }
 
 /*
- * Like sanitize_client_strbuf, but we also perform any canonicalization
+ * Like sanitize_client, but we also perform any canonicalization
  * to make life easier on the admin.
  */
-static void canonicalize_client_strbuf(struct strbuf *out, const char *in)
+static void canonicalize_client(struct strbuf *out, const char *in)
 {
-	sanitize_client_strbuf(out, in);
+	sanitize_client(out, in);
 	strbuf_tolower(out);
 }

@@ -579,10 +579,10 @@ static void parse_host_arg(char *extra_args, int buflen)
 				parse_host_and_port(val, &host, &port);
 				if (port) {
 					strbuf_reset(&tcp_port);
-					sanitize_client_strbuf(&tcp_port, port);
+					sanitize_client(&tcp_port, port);
 				}
 				strbuf_reset(&hostname);
-				canonicalize_client_strbuf(&hostname, host);
+				canonicalize_client(&hostname, host);
 				hostname_lookup_done = 0;
 			}

@@ -620,8 +620,8 @@ static void lookup_hostname(void)
 
 			strbuf_reset(&canon_hostname);
 			if (ai->ai_canonname)
-				sanitize_client_strbuf(&canon_hostname,
-						       ai->ai_canonname);
+				sanitize_client(&canon_hostname,
+						ai->ai_canonname);
 			else
 				strbuf_addbuf(&canon_hostname, &ip_address);

@@ -645,7 +645,7 @@ static void lookup_hostname(void)
 				  addrbuf, sizeof(addrbuf));
 
 			strbuf_reset(&canon_hostname);
-			sanitize_client_strbuf(&canon_hostname, hent->h_name);
+			sanitize_client(&canon_hostname, hent->h_name);
 			strbuf_reset(&ip_address);
 			strbuf_addstr(&ip_address, addrbuf);
 		}

Signature · 1 line

2.3.1

Jeff King

2016-06-15 23:04 UTC #3 lore raw

Quoted 6 lines · René Scharfe

Quoted 3 lines
Not a big deal, but do we want to rename sanitize_client_strbuf to sanitize_client? It only had the unwieldy name to distinguish it from this one.
A patch would look like this. The result is shorter, but no win in terms of vertical space (number of lines).

IMHO this is an improvement, though whether it is enough to merit the code churn I dunno. So I'm in favor, but don't mind dropping it if others disagree.

-Peff

René Scharfe

author

2016-06-15 23:04 UTC #4 lore raw

Am 07.03.2015 um 02:08 schrieb Jeff King:

Quoted 6 lines · René Scharfe

Quoted 5 lines
Quoted 4 lines
Not a big deal, but do we want to rename sanitize_client_strbuf to sanitize_client? It only had the unwieldy name to distinguish it from this one.

Quoted 6 lines

Quoted 2 lines
A patch would look like this. The result is shorter, but no win in terms of vertical space (number of lines).
IMHO this is an improvement, though whether it is enough to merit the code churn I dunno. So I'm in favor, but don't mind dropping it if others disagree.

I don't think the change justifies a separate patch, but we can squash it in. :)

René

René Scharfe

author

2016-06-15 23:04 UTC #5 lore raw

In reply to Jeff King

Am 06.03.2015 um 22:06 schrieb Jeff King:

Quoted 12 lines · René Scharfe

daemon.c static void parse_host_arg(char *extra_args, int buflen) · line 582
Quoted 5 lines
  				if (port) {
daemon.c +2 -2
-					free(tcp_port);
-					tcp_port = sanitize_client(port);
+					strbuf_reset(&tcp_port);
+					sanitize_client_strbuf(&tcp_port, port);
The equivalent of free() is strbuf_release(). I think it is reasonable to strbuf_reset here, since we are about to write into it again anyway (though I doubt it happens much in practice, since that would imply multiple host= segments sent by the client). But later...

Quoted 14 lines

daemon.c static int execute(void) · line 684
Quoted 9 lines
daemon.c +4 -5
-	free(hostname);
-	free(canon_hostname);
-	free(ip_address);
-	free(tcp_port);
-	hostname = canon_hostname = ip_address = tcp_port = NULL;
+	strbuf_reset(&hostname);
+	strbuf_reset(&canon_hostname);
+	strbuf_reset(&ip_address);
+	strbuf_reset(&tcp_port);
These probably want to all be strbuf_release(). Again, I doubt it matters much because this is a forked daemon serving only a single request (so they'll get freed by the OS soon anyway), but I think freeing the memory here follows the original intent.

Using a static strbuf means (in my mind) "don't worry about freeing, a memory leak won't happen anyway because we reuse allocations". The new code adds recycling of allocations, which I somehow expect in connection with static allocations where possible. You're right that using strbuf_release() would match the original code more strictly.

But this block is a no-op anyway because it's the first thing we do to these (initially empty) variables. That's not immediately obvious and should be addressed in a separate patch.

René

Jeff King

2016-06-15 23:04 UTC #6 lore raw

Quoted 11 lines · René Scharfe

Quoted 4 lines
These probably want to all be strbuf_release(). Again, I doubt it matters much because this is a forked daemon serving only a single request (so they'll get freed by the OS soon anyway), but I think freeing the memory here follows the original intent.
Using a static strbuf means (in my mind) "don't worry about freeing, a memory leak won't happen anyway because we reuse allocations". The new code adds recycling of allocations, which I somehow expect in connection with static allocations where possible. You're right that using strbuf_release() would match the original code more strictly.

I don't mind recycling allocations like this. It's just that I think in this case it makes the code actively more confusing, because we don't actually expect any of these allocations to be recycled, do we? We fork+exec for each daemon connection, which should receive exactly one host= parameter.

That being said, I don't mind it too much if the recycling stays here.

Quoted 3 lines

But this block is a no-op anyway because it's the first thing we do to these (initially empty) variables. That's not immediately obvious and should be addressed in a separate patch.

Ah, yeah, just reading the diff I thought this was cleanup, not initialization.

-Peff

René Scharfe

author

2016-06-15 23:04 UTC #7 lore raw

PATCH v2 1/2 daemon: use strbuf for hostname info

In reply to René Scharfe

sanitize_client() is not needed anymore and sanitize_client_strbuf() takes its place and name.

Helped-by Jeff King <[email protected]>

Signed-off-by Rene Scharfe <[email protected]>

---
This one is the first patch + s/_strbuf// + s/_reset/_release/.

 daemon.c | 98 +++++++++++++++++++++++++++-------------------------------------
 1 file changed, 41 insertions(+), 57 deletions(-)

daemon.c +41 ~~-57~~

index c3edd96..265c188 100644
@@ -56,10 +56,10 @@ static const char *user_path;
 static unsigned int timeout;
 static unsigned int init_timeout;
 
-static char *hostname;
-static char *canon_hostname;
-static char *ip_address;
-static char *tcp_port;
+static struct strbuf hostname = STRBUF_INIT;
+static struct strbuf canon_hostname = STRBUF_INIT;
+static struct strbuf ip_address = STRBUF_INIT;
+static struct strbuf tcp_port = STRBUF_INIT;
 
 static int hostname_lookup_done;

@@ -68,13 +68,13 @@ static void lookup_hostname(void);
 static const char *get_canon_hostname(void)
 {
 	lookup_hostname();
-	return canon_hostname;
+	return canon_hostname.buf;
 }
 
 static const char *get_ip_address(void)
 {
 	lookup_hostname();
-	return ip_address;
+	return ip_address.buf;
 }
 
 static void logreport(int priority, const char *err, va_list params)

@@ -122,12 +122,6 @@ static void NORETURN daemon_die(const char *err, va_list params)
 	exit(1);
 }
 
-static void strbuf_addstr_or_null(struct strbuf *sb, const char *s)
-{
-	if (s)
-		strbuf_addstr(sb, s);
-}
-
 struct expand_path_context {
 	const char *directory;
 };

@@ -138,22 +132,22 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
 
 	switch (placeholder[0]) {
 	case 'H':
-		strbuf_addstr_or_null(sb, hostname);
+		strbuf_addbuf(sb, &hostname);
 		return 1;
 	case 'C':
 		if (placeholder[1] == 'H') {
-			strbuf_addstr_or_null(sb, get_canon_hostname());
+			strbuf_addstr(sb, get_canon_hostname());
 			return 2;
 		}
 		break;
 	case 'I':
 		if (placeholder[1] == 'P') {
-			strbuf_addstr_or_null(sb, get_ip_address());
+			strbuf_addstr(sb, get_ip_address());
 			return 2;
 		}
 		break;
 	case 'P':
-		strbuf_addstr_or_null(sb, tcp_port);
+		strbuf_addbuf(sb, &tcp_port);
 		return 1;
 	case 'D':
 		strbuf_addstr(sb, context->directory);

@@ -301,16 +295,14 @@ static int run_access_hook(struct daemon_service *service, const char *dir, cons
 	char *eol;
 	int seen_errors = 0;
 
-#define STRARG(x) ((x) ? (x) : "")
 	*arg++ = access_hook;
 	*arg++ = service->name;
 	*arg++ = path;
-	*arg++ = STRARG(hostname);
-	*arg++ = STRARG(get_canon_hostname());
-	*arg++ = STRARG(get_ip_address());
-	*arg++ = STRARG(tcp_port);
+	*arg++ = hostname.buf;
+	*arg++ = get_canon_hostname();
+	*arg++ = get_ip_address();
+	*arg++ = tcp_port.buf;
 	*arg = NULL;
-#undef STRARG
 
 	child.use_shell = 1;
 	child.argv = argv;

@@ -542,7 +534,7 @@ static void parse_host_and_port(char *hostport, char **host,
  * trailing and leading dots, which means that the client cannot escape
  * our base path via ".." traversal.
  */
-static void sanitize_client_strbuf(struct strbuf *out, const char *in)
+static void sanitize_client(struct strbuf *out, const char *in)
 {
 	for (; *in; in++) {
 		if (*in == '/')

@@ -556,23 +548,14 @@ static void sanitize_client_strbuf(struct strbuf *out, const char *in)
 		strbuf_setlen(out, out->len - 1);
 }
 
-static char *sanitize_client(const char *in)
-{
-	struct strbuf out = STRBUF_INIT;
-	sanitize_client_strbuf(&out, in);
-	return strbuf_detach(&out, NULL);
-}
-
 /*
  * Like sanitize_client, but we also perform any canonicalization
  * to make life easier on the admin.
  */
-static char *canonicalize_client(const char *in)
+static void canonicalize_client(struct strbuf *out, const char *in)
 {
-	struct strbuf out = STRBUF_INIT;
-	sanitize_client_strbuf(&out, in);
-	strbuf_tolower(&out);
-	return strbuf_detach(&out, NULL);
+	sanitize_client(out, in);
+	strbuf_tolower(out);
 }
 
 /*

@@ -595,11 +578,11 @@ static void parse_host_arg(char *extra_args, int buflen)
 				char *port;
 				parse_host_and_port(val, &host, &port);
 				if (port) {
-					free(tcp_port);
-					tcp_port = sanitize_client(port);
+					strbuf_reset(&tcp_port);
+					sanitize_client(&tcp_port, port);
 				}
-				free(hostname);
-				hostname = canonicalize_client(host);
+				strbuf_reset(&hostname);
+				canonicalize_client(&hostname, host);
 				hostname_lookup_done = 0;
 			}

@@ -616,7 +599,7 @@ static void parse_host_arg(char *extra_args, int buflen)
  */
 static void lookup_hostname(void)
 {
-	if (!hostname_lookup_done && hostname) {
+	if (!hostname_lookup_done && hostname.len) {
 #ifndef NO_IPV6
 		struct addrinfo hints;
 		struct addrinfo *ai;

@@ -626,19 +609,21 @@ static void lookup_hostname(void)
 		memset(&hints, 0, sizeof(hints));
 		hints.ai_flags = AI_CANONNAME;
 
-		gai = getaddrinfo(hostname, NULL, &hints, &ai);
+		gai = getaddrinfo(hostname.buf, NULL, &hints, &ai);
 		if (!gai) {
 			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
 
 			inet_ntop(AF_INET, &sin_addr->sin_addr,
 				  addrbuf, sizeof(addrbuf));
-			free(ip_address);
-			ip_address = xstrdup(addrbuf);
+			strbuf_reset(&ip_address);
+			strbuf_addstr(&ip_address, addrbuf);
 
-			free(canon_hostname);
-			canon_hostname = ai->ai_canonname ?
-				sanitize_client(ai->ai_canonname) :
-				xstrdup(ip_address);
+			strbuf_reset(&canon_hostname);
+			if (ai->ai_canonname)
+				sanitize_client(&canon_hostname,
+						ai->ai_canonname);
+			else
+				strbuf_addbuf(&canon_hostname, &ip_address);
 
 			freeaddrinfo(ai);
 		}

@@ -648,7 +633,7 @@ static void lookup_hostname(void)
 		char **ap;
 		static char addrbuf[HOST_NAME_MAX + 1];
 
-		hent = gethostbyname(hostname);
+		hent = gethostbyname(hostname.buf);
 		if (hent) {
 			ap = hent->h_addr_list;
 			memset(&sa, 0, sizeof sa);

@@ -659,10 +644,10 @@ static void lookup_hostname(void)
 			inet_ntop(hent->h_addrtype, &sa.sin_addr,
 				  addrbuf, sizeof(addrbuf));
 
-			free(canon_hostname);
-			canon_hostname = sanitize_client(hent->h_name);
-			free(ip_address);
-			ip_address = xstrdup(addrbuf);
+			strbuf_reset(&canon_hostname);
+			sanitize_client(&canon_hostname, hent->h_name);
+			strbuf_reset(&ip_address);
+			strbuf_addstr(&ip_address, addrbuf);
 		}
 #endif
 		hostname_lookup_done = 1;

@@ -693,11 +678,10 @@ static int execute(void)
 		pktlen--;
 	}
 
-	free(hostname);
-	free(canon_hostname);
-	free(ip_address);
-	free(tcp_port);
-	hostname = canon_hostname = ip_address = tcp_port = NULL;
+	strbuf_release(&hostname);
+	strbuf_release(&canon_hostname);
+	strbuf_release(&ip_address);
+	strbuf_release(&tcp_port);
 
 	if (len != pktlen)
 		parse_host_arg(line + len + 1, pktlen - len - 1);

Signature · 1 line

2.3.1

René Scharfe

author

2016-06-15 23:04 UTC #8 lore raw

PATCH v2 2/2 daemon: deglobalize hostname information

In reply to René Scharfe

Move the variables related to the client-supplied hostname into its own struct, let execute() own an instance of that instead of storing the information in global variables and pass the struct to any function that needs to access it as a parameter.

The lifetime of the variables is easier to see this way. Allocated memory is released within execute(). The strbufs don't have to be reset anymore because they are written to only once at most: parse_host_arg() is only called once by execute() and lookup_hostname() guards against being called twice using hostname_lookup_done.

Signed-off-by Rene Scharfe <[email protected]>

---
 daemon.c | 133 +++++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 74 insertions(+), 59 deletions(-)

daemon.c +74 ~~-59~~

index 265c188..9ee2187 100644
@@ -43,9 +43,6 @@ static const char *base_path;
 static const char *interpolated_path;
 static int base_path_relaxed;
 
-/* Flag indicating client sent extra args. */
-static int saw_extended_args;
-
 /* If defined, ~user notation is allowed and the string is inserted
  * after ~user/.  E.g. a request to git://host/~alice/frotz would
  * go to /home/alice/pub_git/frotz with --user-path=pub_git.

@@ -56,25 +53,27 @@ static const char *user_path;
 static unsigned int timeout;
 static unsigned int init_timeout;
 
-static struct strbuf hostname = STRBUF_INIT;
-static struct strbuf canon_hostname = STRBUF_INIT;
-static struct strbuf ip_address = STRBUF_INIT;
-static struct strbuf tcp_port = STRBUF_INIT;
-
-static int hostname_lookup_done;
+struct hostinfo {
+	struct strbuf hostname;
+	struct strbuf canon_hostname;
+	struct strbuf ip_address;
+	struct strbuf tcp_port;
+	unsigned int hostname_lookup_done:1;
+	unsigned int saw_extended_args:1;
+};
 
-static void lookup_hostname(void);
+static void lookup_hostname(struct hostinfo *hi);
 
-static const char *get_canon_hostname(void)
+static const char *get_canon_hostname(struct hostinfo *hi)
 {
-	lookup_hostname();
-	return canon_hostname.buf;
+	lookup_hostname(hi);
+	return hi->canon_hostname.buf;
 }
 
-static const char *get_ip_address(void)
+static const char *get_ip_address(struct hostinfo *hi)
 {
-	lookup_hostname();
-	return ip_address.buf;
+	lookup_hostname(hi);
+	return hi->ip_address.buf;
 }
 
 static void logreport(int priority, const char *err, va_list params)

@@ -124,30 +123,32 @@ static void NORETURN daemon_die(const char *err, va_list params)
 
 struct expand_path_context {
 	const char *directory;
+	struct hostinfo *hostinfo;
 };
 
 static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
 {
 	struct expand_path_context *context = ctx;
+	struct hostinfo *hi = context->hostinfo;
 
 	switch (placeholder[0]) {
 	case 'H':
-		strbuf_addbuf(sb, &hostname);
+		strbuf_addbuf(sb, &hi->hostname);
 		return 1;
 	case 'C':
 		if (placeholder[1] == 'H') {
-			strbuf_addstr(sb, get_canon_hostname());
+			strbuf_addstr(sb, get_canon_hostname(hi));
 			return 2;
 		}
 		break;
 	case 'I':
 		if (placeholder[1] == 'P') {
-			strbuf_addstr(sb, get_ip_address());
+			strbuf_addstr(sb, get_ip_address(hi));
 			return 2;
 		}
 		break;
 	case 'P':
-		strbuf_addbuf(sb, &tcp_port);
+		strbuf_addbuf(sb, &hi->tcp_port);
 		return 1;
 	case 'D':
 		strbuf_addstr(sb, context->directory);

@@ -156,7 +157,7 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
 	return 0;
 }
 
-static const char *path_ok(const char *directory)
+static const char *path_ok(const char *directory, struct hostinfo *hi)
 {
 	static char rpath[PATH_MAX];
 	static char interp_path[PATH_MAX];

@@ -192,11 +193,12 @@ static const char *path_ok(const char *directory)
 			dir = rpath;
 		}
 	}
-	else if (interpolated_path && saw_extended_args) {
+	else if (interpolated_path && hi->saw_extended_args) {
 		struct strbuf expanded_path = STRBUF_INIT;
 		struct expand_path_context context;
 
 		context.directory = directory;
+		context.hostinfo = hi;
 
 		if (*dir != '/') {
 			/* Allow only absolute */

@@ -286,7 +288,8 @@ static int daemon_error(const char *dir, const char *msg)
 
 static const char *access_hook;
 
-static int run_access_hook(struct daemon_service *service, const char *dir, const char *path)
+static int run_access_hook(struct daemon_service *service, const char *dir,
+			   const char *path, struct hostinfo *hi)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;

@@ -298,10 +301,10 @@ static int run_access_hook(struct daemon_service *service, const char *dir, cons
 	*arg++ = access_hook;
 	*arg++ = service->name;
 	*arg++ = path;
-	*arg++ = hostname.buf;
-	*arg++ = get_canon_hostname();
-	*arg++ = get_ip_address();
-	*arg++ = tcp_port.buf;
+	*arg++ = hi->hostname.buf;
+	*arg++ = get_canon_hostname(hi);
+	*arg++ = get_ip_address(hi);
+	*arg++ = hi->tcp_port.buf;
 	*arg = NULL;
 
 	child.use_shell = 1;

@@ -346,7 +349,8 @@ error_return:
 	return -1;
 }
 
-static int run_service(const char *dir, struct daemon_service *service)
+static int run_service(const char *dir, struct daemon_service *service,
+		       struct hostinfo *hi)
 {
 	const char *path;
 	int enabled = service->enabled;

@@ -360,7 +364,7 @@ static int run_service(const char *dir, struct daemon_service *service)
 		return daemon_error(dir, "service not enabled");
 	}
 
-	if (!(path = path_ok(dir)))
+	if (!(path = path_ok(dir, hi)))
 		return daemon_error(dir, "no such repository");
 
 	/*

@@ -396,7 +400,7 @@ static int run_service(const char *dir, struct daemon_service *service)
 	 * Optionally, a hook can choose to deny access to the
 	 * repository depending on the phase of the moon.
 	 */
-	if (access_hook && run_access_hook(service, dir, path))
+	if (access_hook && run_access_hook(service, dir, path, hi))
 		return -1;
 
 	/*

@@ -561,14 +565,14 @@ static void canonicalize_client(struct strbuf *out, const char *in)
 /*
  * Read the host as supplied by the client connection.
  */
-static void parse_host_arg(char *extra_args, int buflen)
+static void parse_host_arg(struct hostinfo *hi, char *extra_args, int buflen)
 {
 	char *val;
 	int vallen;
 	char *end = extra_args + buflen;
 
 	if (extra_args < end && *extra_args) {
-		saw_extended_args = 1;
+		hi->saw_extended_args = 1;
 		if (strncasecmp("host=", extra_args, 5) == 0) {
 			val = extra_args + 5;
 			vallen = strlen(val) + 1;

@@ -577,13 +581,10 @@ static void parse_host_arg(char *extra_args, int buflen)
 				char *host;
 				char *port;
 				parse_host_and_port(val, &host, &port);
-				if (port) {
-					strbuf_reset(&tcp_port);
-					sanitize_client(&tcp_port, port);
-				}
-				strbuf_reset(&hostname);
-				canonicalize_client(&hostname, host);
-				hostname_lookup_done = 0;
+				if (port)
+					sanitize_client(&hi->tcp_port, port);
+				canonicalize_client(&hi->hostname, host);
+				hi->hostname_lookup_done = 0;
 			}
 
 			/* On to the next one */

@@ -597,9 +598,9 @@ static void parse_host_arg(char *extra_args, int buflen)
 /*
  * Locate canonical hostname and its IP address.
  */
-static void lookup_hostname(void)
+static void lookup_hostname(struct hostinfo *hi)
 {
-	if (!hostname_lookup_done && hostname.len) {
+	if (!hi->hostname_lookup_done && hi->hostname.len) {
 #ifndef NO_IPV6
 		struct addrinfo hints;
 		struct addrinfo *ai;

@@ -609,21 +610,20 @@ static void lookup_hostname(void)
 		memset(&hints, 0, sizeof(hints));
 		hints.ai_flags = AI_CANONNAME;
 
-		gai = getaddrinfo(hostname.buf, NULL, &hints, &ai);
+		gai = getaddrinfo(hi->hostname.buf, NULL, &hints, &ai);
 		if (!gai) {
 			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
 
 			inet_ntop(AF_INET, &sin_addr->sin_addr,
 				  addrbuf, sizeof(addrbuf));
-			strbuf_reset(&ip_address);
-			strbuf_addstr(&ip_address, addrbuf);
+			strbuf_addstr(&hi->ip_address, addrbuf);
 
-			strbuf_reset(&canon_hostname);
 			if (ai->ai_canonname)
-				sanitize_client(&canon_hostname,
+				sanitize_client(&hi->canon_hostname,
 						ai->ai_canonname);
 			else
-				strbuf_addbuf(&canon_hostname, &ip_address);
+				strbuf_addbuf(&hi->canon_hostname,
+					      &hi->ip_address);
 
 			freeaddrinfo(ai);
 		}

@@ -644,22 +644,39 @@ static void lookup_hostname(void)
 			inet_ntop(hent->h_addrtype, &sa.sin_addr,
 				  addrbuf, sizeof(addrbuf));
 
-			strbuf_reset(&canon_hostname);
-			sanitize_client(&canon_hostname, hent->h_name);
-			strbuf_reset(&ip_address);
-			strbuf_addstr(&ip_address, addrbuf);
+			sanitize_client(&hi->canon_hostname, hent->h_name);
+			strbuf_addstr(&hi->ip_address, addrbuf);
 		}
 #endif
-		hostname_lookup_done = 1;
+		hi->hostname_lookup_done = 1;
 	}
 }
 
+static void hostinfo_init(struct hostinfo *hi)
+{
+	memset(hi, 0, sizeof(*hi));
+	strbuf_init(&hi->hostname, 0);
+	strbuf_init(&hi->canon_hostname, 0);
+	strbuf_init(&hi->ip_address, 0);
+	strbuf_init(&hi->tcp_port, 0);
+}
+
+static void hostinfo_clear(struct hostinfo *hi)
+{
+	strbuf_release(&hi->hostname);
+	strbuf_release(&hi->canon_hostname);
+	strbuf_release(&hi->ip_address);
+	strbuf_release(&hi->tcp_port);
+}
 
 static int execute(void)
 {
 	char *line = packet_buffer;
 	int pktlen, len, i;
 	char *addr = getenv("REMOTE_ADDR"), *port = getenv("REMOTE_PORT");
+	struct hostinfo hi;
+
+	hostinfo_init(&hi);
 
 	if (addr)
 		loginfo("Connection from %s:%s", addr, port);

@@ -678,13 +695,8 @@ static int execute(void)
 		pktlen--;
 	}
 
-	strbuf_release(&hostname);
-	strbuf_release(&canon_hostname);
-	strbuf_release(&ip_address);
-	strbuf_release(&tcp_port);
-
 	if (len != pktlen)
-		parse_host_arg(line + len + 1, pktlen - len - 1);
+		parse_host_arg(&hi, line + len + 1, pktlen - len - 1);
 
 	for (i = 0; i < ARRAY_SIZE(daemon_service); i++) {
 		struct daemon_service *s = &(daemon_service[i]);

@@ -697,10 +709,13 @@ static int execute(void)
 			 * Note: The directory here is probably context sensitive,
 			 * and might depend on the actual service being performed.
 			 */
-			return run_service(arg, s);
+			int rc = run_service(arg, s, &hi);
+			hostinfo_clear(&hi);
+			return rc;
 		}
 	}
 
+	hostinfo_clear(&hi);
 	logerror("Protocol error: '%s'", line);
 	return -1;
 }

Signature · 1 line

2.3.1